Conversation
🦋 Changeset detectedLatest commit: b298a27 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
silesky
force-pushed
the
bug/beforeunload-not-firing-on-safari
branch
from
7bdfc79 to
cd97543
Compare
| @@ -1,6 +1,7 @@ | |||
| import { PriorityQueue } from '.' | |||
There was a problem hiding this comment.
This file is not in use FYI, ended up not needing to bring it into core and will remove it when I get a chance.
| let unloaded = false // prevents double firing if both are supported | ||
|
|
||
| // using document instead of window because of bug affecting browsers before safari 14 (detail in footnotes https://caniuse.com/?search=visibilitychange) | ||
| document.addEventListener('pagehide', () => { |
There was a problem hiding this comment.
document instead of window FYI because of an old event bubbling issue (see comment). This shouldn't cause issues that I an forsee, and it's IMO as a rule of thumb it's better to use document rather than window when possible.
agouil
left a comment
agouil
left a comment
There was a problem hiding this comment.
Code changes look good to me. I suggest adding a test where you fire both events and you test that the cb() is only called once.
| cb() | ||
| }) | ||
|
|
||
| document.addEventListener('visibilitychange', () => { |
There was a problem hiding this comment.
Should we reset the unloaded back to false if the visibilityState is not hidden? That way if someone comes back to the tab we still support the same unload behaviour if they close later on?
Edit: Not sure if that also means we need to rehydrate the events and put them in the queue to ensure they're sent if we return to the existing tab, or if a.js will already just try to resend them.
There was a problem hiding this comment.
Should we reset the unloaded back to false if the visibilityState is not hidden? That way if someone comes back to the tab we still support the same unload behaviour if they close later on?
This is a good call out. I blame StackOverflow =)
Edit: Not sure if that also means we need to rehydrate the events and put them in the queue to ensure they're sent if we return to the existing tab, or if a.js will already just try to resend them.
This is another good call out -- this PR is focused on the batched-dispatcher, which leaves the scenario of events in the main event queue -- as for PriorityQueue, you made me realize we actually don't want to save to localStorage on tab away since everything (including unsent events in the queue) should be suspended in-memory, and PQ. In fact, if I'm following the logic, doing so could lead to potentially duplicate events building up as you'll have the same events both in-memory and in storage.
There was a problem hiding this comment.
Ah awesome, that makes sense then :phew:
silesky
force-pushed
the
bug/beforeunload-not-firing-on-safari
branch
from
fbee515 to
b298a27
Compare
chrisradek
left a comment
chrisradek
left a comment
There was a problem hiding this comment.
Looks great, ship it!
4 participants
This is a bug and enhancement
This hybrid approach is because "beforeunload" is unreliable, and "pagehide" by itself will not fire on tab switch / close. visibilitychange works well, but it doesn't have as much support for older browsers as pagehide, and there are still some bugs that can cause it to not fire. https://stackoverflow.com/questions/3239834/window-onbeforeunload-not-working-on-the-ipad/52864508#52864