Use visibility API instead of beforeunload #158
Conversation
Using the `beforeunload` event prevents browsers from caching the page in the page navigation cache: https://developers.google.com/web/updates/2018/07/page-lifecycle-api#the-beforeunload-event Page Visibility is a better alternative. This API is widely supported except for old Android, where it's prefixed on 4.4 and unavailable on lower versions. I'm happy to add fallbacks if any reviewers feel they're necessary.
|
+1. Would also like to avoid
|
Codecov Report
@@ Coverage Diff @@
## master #158 +/- ##
==========================================
- Coverage 84.37% 82.82% -1.56%
==========================================
Files 7 7
Lines 160 163 +3
Branches 24 25 +1
==========================================
Hits 135 135
- Misses 18 21 +3
Partials 7 7
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #158 +/- ##
==========================================
+ Coverage 84.37% 85.53% +1.15%
==========================================
Files 7 7
Lines 160 159 -1
Branches 24 25 +1
==========================================
+ Hits 135 136 +1
+ Misses 18 17 -1
+ Partials 7 6 -1
Continue to review full report at Codecov.
|
src/browser.js
Outdated
| @@ -50,6 +50,11 @@ class UniversalEmitter extends Emitter { | |||
| from(): UniversalEmitter { | |||
| return this; | |||
| } | |||
| flushBeforeTerminated = () => { | |||
| if (document.hidden) { | |||
| this.flushInternal(); | |||
There was a problem hiding this comment.
Why this.flushInternal() vs this.flush()?
There was a problem hiding this comment.
bind is slow and we already have the this context
|
|
There was a problem hiding this comment.
I think this makes sense to me, and would be good to fix the caching issues. My only question, visibilitychange seems to not fire during a page navigation, where as beforeunload does. Are we now counting on the interval to handle events when the user navigates away?
|
Chrome should be fixed as of M56 (May 2017): Safari WebKit still has an open issue: Unclear status for Edge, but was filed internally: This would definitely be a tradeoff and reduce the logging for IE/Safari to that only captured by the interval until they properly implement |
There was a problem hiding this comment.
Android 4.4 and lower seem to hold around ~10% of android browser market share according to: https://fossbytes.com/most-popular-android-versions-always-updated/
We could add a fallback, but the market share does seem to be dropping quickly enough that I'm not sure the extra cruft is worth it. Thank you for the PR and iteration on testing.
|
!merge |
Using the
beforeunloadevent prevents browsers from caching the pagein the page navigation cache:
https://developers.google.com/web/updates/2018/07/page-lifecycle-api#the-beforeunload-event
Page Visibility is a better alternative. This API is widely supported
except for old Android, where it's prefixed on 4.4 and unavailable on
lower versions. I'm happy to add fallbacks if any reviewers feel they're
necessary.