Skip to content

GAUD-10126: switch from unload to pagehide #41

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dlockhart merged 1 commit into from
Jun 12, 2026
Merged

GAUD-10126: switch from unload to pagehide #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
0 eslint.config.mjs → eslint.config.js
View file
Open in desktop
File renamed without changes.
26 changes: 15 additions & 11 deletions logging.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ export class ServerLogger {
this._batchSize = batchSize;
this._batchTime = batchTime;
this._logs = [];
window.addEventListener('unload', this._onUnload.bind(this));
this._onPageHide = this._onPageHide.bind(this);
window.addEventListener('pagehide', this._onPageHide);

@dlockhart dlockhart Jun 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dlockhart dlockhart Jun 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dbatiste dbatiste Jun 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dlockhart dlockhart Jun 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

logBatch(logs) {
Expand All @@ -36,6 +37,11 @@ export class ServerLogger {
}
}

_disconnectForTesting() {
window.removeEventListener('pagehide', this._onPageHide);
clearTimeout(this._batchTimeout);

@dlockhart dlockhart Jun 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new tests I added were failing sporadically because the old loggers from the other tests were still doing things after their tests finished!

}

async _log(logs) {
let options = {
method: 'POST',
Expand All @@ -58,19 +64,17 @@ export class ServerLogger {
}
}

async _onUnload() {
async _onPageHide() {
clearTimeout(this._batchTimeout);
if (!this._loggerPromise && !navigator || !navigator.sendBeacon) {
if (!this._loggerPromise || !navigator?.sendBeacon || this._logs.length === 0) {

@dlockhart dlockhart Jun 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return;
}
if (this._logs.length > 0) {
try {
const logger = await this._loggerPromise;
const data = JSON.stringify(this._logs);
navigator.sendBeacon(logger.Endpoint, data);
} catch (err) {
console.error(err, this._logs);
}
try {
const logger = await this._loggerPromise;
const data = JSON.stringify(this._logs);
navigator.sendBeacon(logger.Endpoint, data);
} catch (err) {
console.error(err, this._logs);
}
}

Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
"access": "public"
},
"main": "index.js",
"type": "module",

@dlockhart dlockhart Jun 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This project was already ESM, so I don't think this is going to be a problem.

"scripts": {
"lint": "npm run lint:eslint",
"lint:eslint": "eslint .",
Expand All @@ -16,7 +17,6 @@
"author": "D2L Corporation",
"license": "Apache-2.0",
"devDependencies": {
"@babel/core": "^7",

@dlockhart dlockhart Jun 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was doing weird things in the lint output.

"@brightspace-ui/testing": "^1",
"eslint": "^9",
"eslint-config-brightspace": "^2",
Expand Down
32 changes: 25 additions & 7 deletions test/logging.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,10 @@ describe('logging', () => {
consoleErrorStub = stub(console, 'error');
});

afterEach(() => restore());
afterEach(() => {
restore();
logger._disconnectForTesting();
});

describe('batching', () => {

Expand Down Expand Up @@ -862,30 +865,45 @@ describe('logging', () => {

});

describe('unload', () => {
describe('pagehide', () => {
let beaconStub;

beforeEach(() => {
beaconStub = stub(navigator, 'sendBeacon');
});

afterEach(() => restore());

it('should send remaining logs as beacon on unload', async() => {

it('should send remaining logs as beacon when "pagehide" event is triggered', async() => {
logger.logBatch([{ message: '1' }, { message: '2' }]);

window.dispatchEvent(new Event('unload'));
window.dispatchEvent(new Event('pagehide'));

await aTimeout(0);

expect(beaconStub).to.have.been.calledOnce;
expect(beaconStub.calledOnce).to.be.true;

@dlockhart dlockhart Jun 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other format of this expect was blowing up when testing the failure scenario.

const [endpoint, data] = beaconStub.getCall(0).args;

expect(endpoint).to.equal('/test/endpoint/0');
expect(data).to.equal(JSON.stringify([{ message: '1' }, { message: '2' }]));
});

it('should not send anything as beacon if no logs have ever been sent', async() => {
window.dispatchEvent(new Event('pagehide'));
await aTimeout(0);

expect(beaconStub.called).to.be.false;
});

it('should not send anything as beacon if there are no logs', async() => {
logger.logBatch([{ message: '1' }]);
await aTimeout(batchTime);

window.dispatchEvent(new Event('pagehide'));
await aTimeout(0);

expect(beaconStub.called).to.be.false;
});

});

});
Expand Down