GAUD-10126: switch from unload to pagehide#41
Conversation
| "access": "public" | ||
| }, | ||
| "main": "index.js", | ||
| "type": "module", |
There was a problem hiding this comment.
This project was already ESM, so I don't think this is going to be a problem.
| "author": "D2L Corporation", | ||
| "license": "Apache-2.0", | ||
| "devDependencies": { | ||
| "@babel/core": "^7", |
There was a problem hiding this comment.
Was doing weird things in the lint output.
| async _onPageHide() { | ||
| clearTimeout(this._batchTimeout); | ||
| if (!this._loggerPromise && !navigator || !navigator.sendBeacon) { | ||
| if (!this._loggerPromise || !navigator?.sendBeacon || this._logs.length === 0) { |
There was a problem hiding this comment.
So I think this was a bug before -- it should have been !this._loggerPromise || !navigator || !navigator.sendBeach. I added the empty _logs check to reduce the nesting of the code below.
|
|
||
| _disconnectForTesting() { | ||
| window.removeEventListener('pagehide', this._onPageHide); | ||
| clearTimeout(this._batchTimeout); |
There was a problem hiding this comment.
The new tests I added were failing sporadically because the old loggers from the other tests were still doing things after their tests finished!
| await aTimeout(0); | ||
|
|
||
| expect(beaconStub).to.have.been.calledOnce; | ||
| expect(beaconStub.calledOnce).to.be.true; |
There was a problem hiding this comment.
The other format of this expect was blowing up when testing the failure scenario.
| this._logs = []; | ||
| window.addEventListener('unload', this._onUnload.bind(this)); | ||
| this._onPageHide = this._onPageHide.bind(this); | ||
| window.addEventListener('pagehide', this._onPageHide); |
There was a problem hiding this comment.
I specifically didn't feature flag this since our logger both runs in so many places that likely don't have access to our feature flags but also runs so early in the page cycle.
| this._logs = []; | ||
| window.addEventListener('unload', this._onUnload.bind(this)); | ||
| this._onPageHide = this._onPageHide.bind(this); | ||
| window.addEventListener('pagehide', this._onPageHide); |
There was a problem hiding this comment.
I should mention that I spent a lot of time looking into visibilitychange as well, as it's also recommended in the same breath as pagehide for sendBeacon. It was pretty unreliable in Chrome, but also fires every time the user switches tabs, which we don't really care about here.
There was a problem hiding this comment.
The documentation says:
The pagehide event is sent to a Window when the browser hides the current page in the process of presenting a different page from the session's history.
For example, when the user clicks the browser's Back button, the current page receives a pagehide event before the previous page is shown.
I'm guessing pageHide event fires when navigating to a new page not in the user's session history, but this description doesn't make it sound that way.
There was a problem hiding this comment.
It does yes. Following a link (both on the same domain or another), reloading the current page, as well as normal back/forward all trigger it.
|
🎉 This PR is included in version 1.9.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
5 participants
This switches our logger to use the
pagehideevent in the wake of Google dropping support for unload.Testing this was pretty tricky since Chrome has a tendency to run the code but not any
console.logs or show anything in the network traffic. I did confirm though that errors ended up getting logged on back/forward, reload and link clicking in Firefox, Chrome, Edge and Safari.