Skip to content
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

Jump to bottom

Instrument back/forward cache navigations #70

Merged
merged 7 commits into from
Apr 12, 2023
Merged

Instrument back/forward cache navigations #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4304142
Allow tests to opt out of no-cache headers
Mar 29, 2023
f1f0b89
Support more than one route HTML file per test
Mar 29, 2023
363f9f2
New e2e test case: back/forward cache behaviour
Mar 29, 2023
ba2bc2d
Move pageshow listener into the ttvc library
Mar 30, 2023
a47bdd1
Capture navigationType for the measurement in question
Mar 31, 2023
fe35e26
Extend browser's builtin navigation type
Apr 11, 2023
9519aa0
Clarify relationship between navigationType and NavigationTimingType …
Apr 11, 2023
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
17 changes: 17 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,23 @@ export type Metric = {
// (this can be either a mutation or a load event target, whichever
// occurred last)
lastVisibleChange?: HTMLElement | TimestampedMutationRecord;

// describes how the navigation being measured was initiated
// NOTE: this extends the navigation type values defined in the W3 spec;
// "script" is usually reported as "navigation" by the browser, but we
// report that distinctly
// @see https://developer.mozilla.org/en-US/docs/Web/API/PerformanceNavigationTiming/type
navigationType: // Navigation started by clicking a link, by entering the
// URL in the browser's address bar, or by form submission.
| 'navigate'
// Navigation is through the browser's reload operation.
| 'reload'
// Navigation is through the browser's history traversal operation.
| 'back_forward'
// Navigation is initiated by a prerender hint.
| 'prerender'
// Navigation is triggered with a script operation, e.g. in a single page application.
| 'script';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we document that this is an extension of NavigationTimingType and script is the only additional value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good call, updated.

};
};
```
Expand Down
11 changes: 10 additions & 1 deletion src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,16 @@ export const init = (options?: TtvcOptions) => {

calculator = getVisuallyCompleteCalculator();
void calculator.start();
window.addEventListener('locationchange', () => void calculator.start(performance.now()));

// restart measurement for SPA navigation
window.addEventListener('locationchange', (event) => void calculator.start(event.timeStamp));
Copy link

Choose a reason for hiding this comment

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

Is there more documentation on the locationchange event? I can't find it on MDN or in our repo

Copy link

Choose a reason for hiding this comment

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

Nvm - just realized we have it documented in README already.


// restart measurement on back/forward cache page restoration
window.addEventListener('pageshow', (event) => {
// abort if this is the initial pageload
if (!event.persisted) return;
void calculator.start(event.timeStamp, true);
Copy link

Choose a reason for hiding this comment

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

Is it possible that pageshow and locationchange are both triggered somewhat simultaneously for a page, resulting in sometimes conflicting calls (in an unexpected order) to calculator.start?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that's likely. This event should only be triggered either on initial pageload, in which case we ignore it, or on restore from the bfcache, in which case, nothing else on the page knows that something different from normal operation has occurred.

The design of @dropbox/ttvc also tries to ensure that duplicate, simultaneous calls to start() are effectively idempotent. i.e. the first measurement will be immediately thrown out and only the subsequent value will be used. If this does occur, it shouldn't impact results.

});
};

/**
Expand Down
19 changes: 18 additions & 1 deletion src/visuallyCompleteCalculator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ import {requestAllIdleCallback} from './requestAllIdleCallback';
import {InViewportImageObserver} from './inViewportImageObserver';
import {Logger} from './util/logger';

export type NavigationType =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this extend NavigationTimingType, so we only need to add script?

| NavigationTimingType
// Navigation was triggered with a script operation, e.g. in a single page application.
| 'script';

export type Metric = {
// time since timeOrigin that the navigation was triggered
// (this will be 0 for the initial pageload)
Expand All @@ -22,6 +27,8 @@ export type Metric = {

// the most recent visual update; this can be either a mutation or a load event target
lastVisibleChange?: HTMLElement | TimestampedMutationRecord;

navigationType: NavigationType;
};
};

Expand Down Expand Up @@ -82,7 +89,7 @@ class VisuallyCompleteCalculator {
}

/** begin measuring a new navigation */
async start(start = 0) {
async start(start = 0, isBfCacheRestore = false) {
const navigationIndex = (this.navigationCount += 1);
this.activeMeasurementIndex = navigationIndex;
Logger.info('VisuallyCompleteCalculator.start()');
Expand Down Expand Up @@ -115,12 +122,22 @@ class VisuallyCompleteCalculator {
// identify timestamp of last visible change
const end = Math.max(start, this.lastImageLoadTimestamp, this.lastMutation?.timestamp ?? 0);

const navigationEntries = performance.getEntriesByType(
'navigation'
) as PerformanceNavigationTiming[];
const navigationType = isBfCacheRestore
? 'back_forward'
: start !== 0
? 'script'
: navigationEntries[navigationEntries.length - 1].type;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it matters much either way since there is only 1 navigation entry, but maybe navigationEntries[0] is easier to read?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I wasn't confident that there can only ever be one navigation entry.

It seems likely there are some edge cases where the navigation entry can be reported again, after a cache restore, for instance.

Do you know if the spec guarantees that there will only ever be one navigation entry?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This says

Only the current document is included, so usually there is only one PerformanceNavigationTiming object to observe.

and web-vitals only takes the first navigation entry: https://github.com/GoogleChrome/web-vitals/blob/main/src/lib/getNavigationEntry.ts


// report result to subscribers
this.next({
start,
end,
duration: end - start,
detail: {
navigationType,
didNetworkTimeOut,
lastVisibleChange:
this.lastImageLoadTimestamp > (this.lastMutation?.timestamp ?? 0)
Expand Down
8 changes: 8 additions & 0 deletions test/e2e/bfcache/about.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<head>
<script src="/dist/index.min.js"></script>
<script src="/analytics.js"></script>
</head>

<body>
<h1 id="h1">About</h1>
</body>
8 changes: 8 additions & 0 deletions test/e2e/bfcache/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<head>
<script src="/dist/index.min.js"></script>
<script src="/analytics.js"></script>
</head>

<body>
<h1 id="h1">Hello world!</h1>
</body>
41 changes: 41 additions & 0 deletions test/e2e/bfcache/index.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import {test, expect} from '@playwright/test';

import {FUDGE} from '../../util/constants';
import {getEntries} from '../../util/entries';

const PAGELOAD_DELAY = 1000;

test.describe('TTVC', () => {
test('a static HTML document', async ({page}) => {
await page.goto(`/test/bfcache?delay=${PAGELOAD_DELAY}&cache=true`, {
waitUntil: 'networkidle',
});

let entries = await getEntries(page);

expect(entries.length).toBe(1);
expect(entries[0].duration).toBeGreaterThanOrEqual(PAGELOAD_DELAY);
expect(entries[0].duration).toBeLessThanOrEqual(PAGELOAD_DELAY + FUDGE);
expect(entries[0].detail.navigationType).toBe('navigate');

await page.goto(`/test/bfcache/about?delay=${PAGELOAD_DELAY}&cache=true`, {
waitUntil: 'networkidle',
});

entries = await getEntries(page);

expect(entries.length).toBe(1);
expect(entries[0].duration).toBeGreaterThanOrEqual(PAGELOAD_DELAY);
expect(entries[0].duration).toBeLessThanOrEqual(PAGELOAD_DELAY + FUDGE);
expect(entries[0].detail.navigationType).toBe('navigate');

await page.goBack({waitUntil: 'networkidle'});

entries = await getEntries(page);

// note: webkit clears previous values from this list on page restore
expect(entries[entries.length - 1].duration).toBeGreaterThanOrEqual(0);
expect(entries[entries.length - 1].duration).toBeLessThanOrEqual(FUDGE);
expect(entries[entries.length - 1].detail.navigationType).toBe('back_forward');
});
});
13 changes: 8 additions & 5 deletions test/server/server.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@ const PORT = process.env.PORT ?? 3000;
const app = express();

// disable browser cache
app.use((req, res, next) => {
res.header('Cache-Control', 'no-cache');
res.header('Vary', '*'); // macOS safari doesn't respect Cache-Control
app.use(({query}, res, next) => {
if (!query?.cache) {
res.header('Cache-Control', 'no-cache');
res.header('Vary', '*'); // macOS safari doesn't respect Cache-Control
}
next();
});

Expand Down Expand Up @@ -47,9 +49,10 @@ app.post('/api', (req, res) => {
res.json(req.body);
});

app.get('/test/:view', ({params}, res) => {
app.get('/test/:view/:route?', ({params}, res) => {
const view = params.view;
res.sendFile(`test/e2e/${view}/index.html`, {root: '.'});
const route = params.route ?? 'index';
res.sendFile(`test/e2e/${view}/${route}.html`, {root: '.'});
});

app.listen(PORT, () => {
Expand Down