Skip to content

Commit

Permalink
Address review feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
philipwalton committed Nov 14, 2022
1 parent c3cd48e commit 4eea458
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 34 deletions.
28 changes: 12 additions & 16 deletions src/onTTFB.ts
Expand Up @@ -69,26 +69,22 @@ export const onTTFB = (onReport: ReportCallback, opts?: ReportOpts) => {
if (navEntry) {
const responseStart = navEntry.responseStart;

// In some cases the value reported is negative or is larger
// than the current page time. Ignore these cases:
// In some cases no value is reported by the browser (for
// privacy/security reasons), and in other cases (bugs) the value is
// negative or is larger than the current page time. Ignore these cases:
// https://github.com/GoogleChrome/web-vitals/issues/137
// https://github.com/GoogleChrome/web-vitals/issues/162
if (responseStart < 0 || responseStart > performance.now()) return;
// https://github.com/GoogleChrome/web-vitals/issues/275
if (responseStart <= 0 || responseStart > performance.now()) return;

// If the navigation entry's `responseStart` value is 0, ignore it.
// This likely means the request included a cross-origin redirect, and
// the browser has removed timing info for privacy/security reasons.
// See: https://github.com/GoogleChrome/web-vitals/issues/275
if (responseStart > 0) {
// The activationStart reference is used because TTFB should be
// relative to page activation rather than navigation start if the
// page was prerendered. But in cases where `activationStart` occurs
// after the first byte is received, this time should be clamped at 0.
metric.value = Math.max(responseStart - getActivationStart(), 0);
// The activationStart reference is used because TTFB should be
// relative to page activation rather than navigation start if the
// page was prerendered. But in cases where `activationStart` occurs
// after the first byte is received, this time should be clamped at 0.
metric.value = Math.max(responseStart - getActivationStart(), 0);

metric.entries = [navEntry];
report(true);
}
metric.entries = [navEntry];
report(true);

// Only report TTFB after bfcache restores if a `navigation` entry
// was reported for the initial load.
Expand Down
34 changes: 16 additions & 18 deletions test/e2e/onTTFB-test.js
Expand Up @@ -185,30 +185,28 @@ describe('onTTFB()', async function() {
assert.strictEqual(ttfb2.entries.length, 0);
});

it('ignores navigations with no responseStart timestamp', async function() {
await browser.url('/test/ttfb?responseStart=0');
it('ignores navigations with invalid responseStart timestamps', async function() {
for (const rs of [-1, 0, 1e12]) {
await browser.url(`/test/ttfb?responseStart=${rs}`);

await domReadyState('complete');
await domReadyState('complete');

// Wait a bit to ensure no beacons were sent.
await browser.pause(1000);
// Wait a bit to ensure no beacons were sent.
await browser.pause(1000);

const loadBeacons = await getBeacons();
assert.strictEqual(loadBeacons.length, 0);
const loadBeacons = await getBeacons();
assert.strictEqual(loadBeacons.length, 0);

// Test back-forward navigations to ensure they're sent, even if the
// initial page TTFB value is ignored.
await stubForwardBack();
// Test back-forward navigations to ensure they're not sent either
// in these situations.
await stubForwardBack();

const ttfb = await getTTFBBeacon();
// Wait a bit to ensure no beacons were sent.
await browser.pause(1000);

assert(ttfb.id.match(/^v3-\d+-\d+$/));
assert.strictEqual(ttfb.value, 0);
assert.strictEqual(ttfb.name, 'TTFB');
assert.strictEqual(ttfb.value, ttfb.delta);
assert.strictEqual(ttfb.rating, 'good');
assert.strictEqual(ttfb.navigationType, 'back-forward-cache');
assert.strictEqual(ttfb.entries.length, 0);
const bfcacheBeacons = await getBeacons();
assert.strictEqual(bfcacheBeacons.length, 0);
}
});

describe('attribution', function() {
Expand Down

0 comments on commit 4eea458

Please sign in to comment.