From 8d238cd3405621fd169a37ece69462f5a4bd930f Mon Sep 17 00:00:00 2001 From: Karl-Aksel Puulmann Date: Wed, 22 Jan 2025 11:30:23 +0200 Subject: [PATCH] Scroll depth: Register `pageleave` listener earlier (#5000) * Register `pageleave` listener earlier Currently, pageleave listener is only registered when a successful response is received from plausible API. After this change pageleave listener is registered immediately when pageview is triggered, hopefully increasing capture rate. * Codespell --- .codespellignore | 1 + tracker/src/plausible.js | 19 ++++++------ tracker/test/file-downloads.spec.js | 2 +- tracker/test/fixtures/manual.html | 6 ++-- .../fixtures/pageleave-pageview-props.html | 8 +++++ tracker/test/fixtures/pageleave.html | 1 + tracker/test/pageleave.spec.js | 29 ++++++++++++++--- tracker/test/support/test-utils.js | 31 ++++++++++++++----- 8 files changed, 72 insertions(+), 25 deletions(-) diff --git a/.codespellignore b/.codespellignore index cb20144e71..23d01f85ba 100644 --- a/.codespellignore +++ b/.codespellignore @@ -6,3 +6,4 @@ taht referer referers statics +firs diff --git a/tracker/src/plausible.js b/tracker/src/plausible.js index ad23e290b4..ca96894a2e 100644 --- a/tracker/src/plausible.js +++ b/tracker/src/plausible.js @@ -212,6 +212,15 @@ payload.h = 1 {{/if}} + {{#if pageleave}} + if (isPageview) { + currentPageLeaveIgnored = false + currentPageLeaveURL = payload.u + currentPageLeaveProps = payload.p + registerPageLeaveListener() + } + {{/if}} + var request = new XMLHttpRequest(); request.open('POST', endpoint, true); request.setRequestHeader('Content-Type', 'text/plain'); @@ -220,14 +229,6 @@ request.onreadystatechange = function() { if (request.readyState === 4) { - {{#if pageleave}} - if (isPageview) { - currentPageLeaveIgnored = false - currentPageLeaveURL = payload.u - currentPageLeaveProps = payload.p - registerPageLeaveListener() - } - {{/if}} options && options.callback && options.callback({status: request.status}) } } @@ -246,7 +247,7 @@ {{#unless hash}} if (lastPage === location.pathname) return; {{/unless}} - + {{#if pageleave}} if (isSPANavigation && listeningPageLeave) { triggerPageLeave(); diff --git a/tracker/test/file-downloads.spec.js b/tracker/test/file-downloads.spec.js index 5bac615d3b..a0dc3a131e 100644 --- a/tracker/test/file-downloads.spec.js +++ b/tracker/test/file-downloads.spec.js @@ -45,7 +45,7 @@ test.describe('file-downloads extension', () => { await page.goto('/file-download.html') const downloadURL = LOCAL_SERVER_ADDR + '/' + await page.locator('#local-download').getAttribute('href') - const downloadRequestMockList = mockManyRequests(page, downloadURL, 2) + const downloadRequestMockList = mockManyRequests({ page, path: downloadURL, numberOfRequests: 2 }) await page.click('#local-download') expect((await downloadRequestMockList).length).toBe(1) diff --git a/tracker/test/fixtures/manual.html b/tracker/test/fixtures/manual.html index de07061dc4..f1058c95ef 100644 --- a/tracker/test/fixtures/manual.html +++ b/tracker/test/fixtures/manual.html @@ -26,16 +26,16 @@ diff --git a/tracker/test/fixtures/pageleave-pageview-props.html b/tracker/test/fixtures/pageleave-pageview-props.html index 51aee537ed..c2c9cd11e3 100644 --- a/tracker/test/fixtures/pageleave-pageview-props.html +++ b/tracker/test/fixtures/pageleave-pageview-props.html @@ -11,6 +11,14 @@ Navigate away + + diff --git a/tracker/test/fixtures/pageleave.html b/tracker/test/fixtures/pageleave.html index bb94b92ee2..a8d415e62f 100644 --- a/tracker/test/fixtures/pageleave.html +++ b/tracker/test/fixtures/pageleave.html @@ -11,6 +11,7 @@ Navigate away + Navigate to pageleave-pageview props diff --git a/tracker/test/pageleave.spec.js b/tracker/test/pageleave.spec.js index e4b17809f6..b6cba45cd8 100644 --- a/tracker/test/pageleave.spec.js +++ b/tracker/test/pageleave.spec.js @@ -128,7 +128,7 @@ test.describe('pageleave extension', () => { action: () => page.goto('/pageleave-pageview-props.html'), expectedRequests: [{n: 'pageview', p: {author: 'John'}}] }) - + await expectPlausibleInAction(page, { action: () => page.click('#navigate-away'), expectedRequests: [{n: 'pageleave', p: {author: 'John'}}] @@ -148,9 +148,9 @@ test.describe('pageleave extension', () => { {n: 'pageview', p: {author: 'john'}} ] }) - + await pageleaveCooldown(page) - + await expectPlausibleInAction(page, { action: () => page.click('#jane-post'), expectedRequests: [ @@ -169,4 +169,25 @@ test.describe('pageleave extension', () => { ] }) }) -}) \ No newline at end of file + + test('sends a pageleave when plausible API is slow and user navigates away before response is received', async ({ page }) => { + await expectPlausibleInAction(page, { + action: () => page.goto('/pageleave.html'), + expectedRequests: [{n: 'pageview', u: `${LOCAL_SERVER_ADDR}/pageleave.html`}] + }) + + await expectPlausibleInAction(page, { + action: async () => { + await page.click('#to-pageleave-pageview-props') + await page.click('#back-button-trigger') + }, + expectedRequests: [ + {n: 'pageleave', u: `${LOCAL_SERVER_ADDR}/pageleave.html`}, + {n: 'pageview', u: `${LOCAL_SERVER_ADDR}/pageleave-pageview-props.html`, p: {author: 'John'}}, + {n: 'pageleave', u: `${LOCAL_SERVER_ADDR}/pageleave-pageview-props.html`, p: {author: 'John'}}, + {n: 'pageview', u: `${LOCAL_SERVER_ADDR}/pageleave.html`} + ], + responseDelay: 1000 + }) + }) +}) diff --git a/tracker/test/support/test-utils.js b/tracker/test/support/test-utils.js index 197a99522a..d0220237f2 100644 --- a/tracker/test/support/test-utils.js +++ b/tracker/test/support/test-utils.js @@ -32,13 +32,16 @@ exports.metaKey = function() { // Mocks a specified number of HTTP requests with given path. Returns a promise that resolves to a // list of requests as soon as the specified number of requests is made, or 3 seconds has passed. -const mockManyRequests = function(page, path, numberOfRequests) { +const mockManyRequests = function({ page, path, numberOfRequests, responseDelay }) { return new Promise((resolve, _reject) => { let requestList = [] const requestTimeoutTimer = setTimeout(() => resolve(requestList), 3000) - page.route(path, (route, request) => { + page.route(path, async (route, request) => { requestList.push(request) + if (responseDelay) { + await delay(responseDelay) + } if (requestList.length === numberOfRequests) { clearTimeout(requestTimeoutTimer) resolve(requestList) @@ -53,14 +56,14 @@ exports.mockManyRequests = mockManyRequests /** * A powerful utility function that makes it easy to assert on the event * requests that should or should not have been made after doing a page - * action (e.g. navigating to the page, clicking a page element, etc). + * action (e.g. navigating to the page, clicking a page element, etc). * * @param {Page} page - The Playwright Page object. * @param {Object} args - The object configuring the action and related expectations. * @param {Function} args.action - A function that returns a promise. The function is called * without arguments, and is `await`ed. This is the action that should or should not trigger * Plausible requests on the page. - * @param {Array} [args.expectedRequests] - A list of partial JSON payloads that get matched + * @param {Array} [args.expectedRequests] - A list of partial JSON payloads that get matched * against the bodies of event requests made. An `expectedRequest` is considered as having * occurred if all of its key-value pairs are found from the JSON body of an event request * that was made. The default value is `[]` @@ -73,18 +76,26 @@ exports.mockManyRequests = mockManyRequests * is `expectedRequests.length + refutedRequests.length`. * @param {number} [args.expectedRequestCount] - When provided, expects the total amount of * event requests made to match this number. + * @param {number} [args.responseDelay] - When provided, delays the response from the Plausible + * API by the given number of milliseconds. */ exports.expectPlausibleInAction = async function (page, { action, expectedRequests = [], refutedRequests = [], awaitedRequestCount, - expectedRequestCount + expectedRequestCount, + responseDelay }) { const requestsToExpect = expectedRequestCount ? expectedRequestCount : expectedRequests.length const requestsToAwait = awaitedRequestCount ? awaitedRequestCount : requestsToExpect + refutedRequests.length - - const plausibleRequestMockList = mockManyRequests(page, '/api/event', requestsToAwait) + + const plausibleRequestMockList = mockManyRequests({ + page, + path: '/api/event', + numberOfRequests: requestsToAwait, + responseDelay: responseDelay + }) await action() const requestBodies = (await plausibleRequestMockList).map(r => r.postDataJSON()) @@ -113,7 +124,7 @@ exports.expectPlausibleInAction = async function (page, { const refutedBodySubsetsErrorMessage = `The following requests were made, but were not expected:\n\n${JSON.stringify(refutedButFoundRequestBodies, null, 4)}` expect(refutedButFoundRequestBodies, refutedBodySubsetsErrorMessage).toHaveLength(0) - + expect(requestBodies.length).toBe(requestsToExpect) } @@ -137,3 +148,7 @@ function areFlatObjectsEqual(obj1, obj2) { return keys1.every(key => obj2[key] === obj1[key]) } + +function delay(ms) { + return new Promise(resolve => setTimeout(resolve, ms)) +}