poorejc commented on issue #50: URL: https://github.com/apache/incubator-flagon-useralejs/issues/50#issuecomment-813493982
Web-workers seem like a viable approach on its face. if @confusingstraw sees that it’s working, maybe it has legs—testing is a consensus game. One thing that might be affecting things is if you have npm modules installed globally that aren’t getting integrated locally. Usually, package-lock solves that, but sometimes not (my experience). I did test a direct pull of your branch a few nights ago and saw the same issues with unit test I reported out in the merged version. Could be global/local module conflicts with some of the new packages in jsdom. I build exclusively from package files and don’t maintain any global modules. Also, what version of node.js are you using? I might be a minor version or two behind ‘latest', but I am running 15.x in testing (though didn’t see any LIFECYCLE error codes). Regardless, I’ll update to latest npm and test again. Also, happy to test again on whatever version you are. When you run tests in the merged branch UncleGedd-sendOnClose, are you seeing test failures? In observing logs, which kinds of events were you seeing in your testing that gave you confidence in your solution? Maybe I wasn’t clear in my previous comments, but i noticed that .src/attachHandlers code for refresh events still includes a sendOnRefresh function I hacked together: https://github.com/UncleGedd/incubator-flagon-useralejs/blob/f508dd95b69d129f68350e3f818ba3f28ad3fe0c/src/attachHandlers.js#L149-L154 <https://github.com/UncleGedd/incubator-flagon-useralejs/blob/f508dd95b69d129f68350e3f818ba3f28ad3fe0c/src/attachHandlers.js#L149-L154> If you were relying on ’submit’ events as proof of life for web-worker sendOnClose behavior, sendOnRefresh might have been doing the work you thought sendOnClose was. You’ll notice I took out that function in the original flagon-userale-50 branch and in the new branch I pushed with your commits (UncleGedd-sendOnClose). Overall, my proposal is this: let’s keep testing web-workers until we have consensus in testing (tests (unit/integration). If that doesn’t work we can explore web-sockets. However, right now navigator.sendBeacon is reliably passing integration tests. If we can mock a working unit test for that, we can release that as a patch in the next version (2.2.0). It is a less-than-ideal method. However, it’s working now (issues are theoretical and not yet observable). Point is: what we have now in 2.1.1 doesn’t work at all. navigator.sendBeacon will be more reliable than that. So keep testing web-workers, polish and push navigator.sendBeacon so we can close the open bug now, push navigator.sendBeacon in next release (unless breakthrough in web-worker/web-socket). Then, roll in a more elegant solution (web-worker/web-socket) in future release (2.2.1/2.3.0). Thoughts on this fake-it-till-you-make it solution? > On Apr 3, 2021, at 8:48 AM, UncleGedd ***@***.***> wrote: > > > Thanks for the thorough testing! When I pushed up the branch all tests were passing so it may have been some inconsistency in our environments that was causing failing tests on your end. It is certainly strange that the web worker's network request appears in the dev tools but not in ELK. I'm able to consistently get the expected logs sent to the example app, but I haven't tried with a local ELK. > > Nevertheless, I'm certainly not married to the web worker solution, sounds like it's pretty brittle. I may try another spike using service workers. Like Rob said above, they are supposed to exist regardless of what page is open. > > Let me know if I can be of any assistance on the sendBeacon solution. That sounds like the best way forward. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub <https://github.com/apache/incubator-flagon-useralejs/issues/50#issuecomment-812860922>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC474BTTCMEOAJKPG67XE7TTG4FAHANCNFSM4Y2SOMLA>. > -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
