[CI 3890] switch beforeunload with visibilitychange #346
Conversation
| if (this.options && this.options.trackingSendDelay === 0) { | ||
| this.sendEvents(); | ||
| } else { | ||
| // Defer sending of events to give beforeunload time to register (avoids race condition) |
There was a problem hiding this comment.
What happened/happens when beforeunload/visibilitychange registers?
There was a problem hiding this comment.
beforeunload/visibilitychange changes the pageUnloading value and if the page is unloading than we wouldn't send events and wait for the next page to send those events from the
jjl014
left a comment
jjl014
left a comment
There was a problem hiding this comment.
Thanks for working on this change!
The changes make sense to me, but I also left a comment around the visibilitychange listener. Let me know what you think. Would love to @esezen's thought as well since he mentioned the hidden change previously.
It seems like the following tests are failing as well. 🤔
| helpers.addEventListener('beforeunload', () => { | ||
| this.pageUnloading = true; | ||
| helpers.addEventListener('visibilitychange', () => { | ||
| if (document.visibilityState === 'hidden') { |
There was a problem hiding this comment.
visibilityState=hidden based on what MDN says can mean the "document is either a background tab or part of a minimized window, or the OS screen lock is active".
Would it make sense to add in some additional logic here to call send() if the page becomes visible again? 🤔
if (document.visibilityState === 'visibile') {
this.send();
}
There was a problem hiding this comment.
Sorry I somehow completely missed the notification for this one. I don't see any reason not to. Good call
jjl014
left a comment
jjl014
left a comment
There was a problem hiding this comment.
The changes look good from what I can tell. I am slightly hesitant about releasing this to all beacon customers at the same time though.
Have we tested this out in the autocomplete-ui repo yet? I believe we should be able to install this version of the library by pointing it to this specific branch (can do this locally as well).
If things are working fine and there are no issues with events firing, then we should probably be okay to move forward. We'll want to keep an eye out on the events after deploying the changes.
jjl014
deleted the
ci-3890-client-javascript-autocomplete-ui-research-replacing
branch
Loom showing visibilityChange event firing as expected
https://www.loom.com/share/f4e8b2c2c1a047c09596c3bc819480f6