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

Make bindReporter generic over metric type #359

Merged
merged 2 commits into from
Jul 7, 2023
Merged

Make bindReporter generic over metric type #359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d818bcd
make bindReporter generic over metric type
brendankenny Jun 16, 2023
591e059
switch type names back, restore MetricWithAttribution
brendankenny Jun 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/lib/bindReporter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,12 @@
* limitations under the License.
*/

import {Metric, MetricRatingThresholds, ReportCallback} from '../types.js';
import {MetricType, MetricRatingThresholds} from '../types.js';

const getRating = (
value: number,
thresholds: MetricRatingThresholds
): Metric['rating'] => {
): MetricType['rating'] => {
if (value > thresholds[1]) {
return 'poor';
}
Expand All @@ -29,9 +29,9 @@ const getRating = (
return 'good';
};

export const bindReporter = (
callback: ReportCallback,
metric: Metric,
export const bindReporter = <MetricName extends MetricType['name']>(
callback: (metric: Extract<MetricType, {name: MetricName}>) => void,
metric: Extract<MetricType, {name: MetricName}>,
thresholds: MetricRatingThresholds,
reportAllChanges?: boolean
) => {
Expand Down
18 changes: 12 additions & 6 deletions src/lib/initMetric.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,14 @@ import {getBFCacheRestoreTime} from './bfcache.js';
import {generateUniqueID} from './generateUniqueID.js';
import {getActivationStart} from './getActivationStart.js';
import {getNavigationEntry} from './getNavigationEntry.js';
import {Metric} from '../types.js';
import {MetricType} from '../types.js';

export const initMetric = (name: Metric['name'], value?: number): Metric => {
export const initMetric = <MetricName extends MetricType['name']>(
name: MetricName,
value?: number
) => {
const navEntry = getNavigationEntry();
let navigationType: Metric['navigationType'] = 'navigate';
let navigationType: MetricType['navigationType'] = 'navigate';

if (getBFCacheRestoreTime() >= 0) {
navigationType = 'back-forward-cache';
Expand All @@ -35,16 +38,19 @@ export const initMetric = (name: Metric['name'], value?: number): Metric => {
navigationType = navEntry.type.replace(
/_/g,
'-'
) as Metric['navigationType'];
) as MetricType['navigationType'];
}
}

// Use `entries` type specific for the metric.
const entries: Extract<MetricType, {name: MetricName}>['entries'] = [];

return {
name,
value: typeof value === 'undefined' ? -1 : value,
rating: 'good', // Will be updated if the value changes.
rating: 'good' as const, // If needed, will be updated when reported. `const` to keep the type from widening to `string`.
delta: 0,
tunetheweb marked this conversation as resolved.
Show resolved Hide resolved
entries: [],
entries,
id: generateUniqueID(),
navigationType,
};
Expand Down
5 changes: 2 additions & 3 deletions src/lib/polyfills/interactionCountPolyfill.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
*/

import {observe} from '../observe.js';
import {Metric} from '../../types.js';

declare global {
interface Performance {
Expand All @@ -27,8 +26,8 @@ let interactionCountEstimate = 0;
let minKnownInteractionId = Infinity;
let maxKnownInteractionId = 0;

const updateEstimate = (entries: Metric['entries']) => {
(entries as PerformanceEventTiming[]).forEach((e) => {
const updateEstimate = (entries: PerformanceEventTiming[]) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

observe() checks the callback type, might as well trust it :)

entries.forEach((e) => {
if (e.interactionId) {
minKnownInteractionId = Math.min(minKnownInteractionId, e.interactionId);
maxKnownInteractionId = Math.max(maxKnownInteractionId, e.interactionId);
Expand Down
8 changes: 3 additions & 5 deletions src/onCLS.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import {
CLSMetric,
CLSReportCallback,
MetricRatingThresholds,
ReportCallback,
ReportOpts,
} from './types.js';

Expand Down Expand Up @@ -66,9 +65,8 @@ export const onCLS = (onReport: CLSReportCallback, opts?: ReportOpts) => {
let report: ReturnType<typeof bindReporter>;

let sessionValue = 0;
let sessionEntries: PerformanceEntry[] = [];
let sessionEntries: LayoutShift[] = [];
Copy link
Member Author

Choose a reason for hiding this comment

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

minor type bug that's an explicit error now with these changes (on sessionEntries.push(entry) below), but no functional change


// const handleEntries = (entries: Metric['entries']) => {
const handleEntries = (entries: LayoutShift[]) => {
entries.forEach((entry) => {
// Only count layout shifts without recent user input.
Expand Down Expand Up @@ -106,7 +104,7 @@ export const onCLS = (onReport: CLSReportCallback, opts?: ReportOpts) => {
const po = observe('layout-shift', handleEntries);
if (po) {
report = bindReporter(
onReport as ReportCallback,
onReport,
metric,
CLSThresholds,
opts!.reportAllChanges
Expand All @@ -123,7 +121,7 @@ export const onCLS = (onReport: CLSReportCallback, opts?: ReportOpts) => {
sessionValue = 0;
metric = initMetric('CLS', 0);
report = bindReporter(
onReport as ReportCallback,
onReport,
metric,
CLSThresholds,
opts!.reportAllChanges
Expand Down
5 changes: 2 additions & 3 deletions src/onFCP.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import {
FCPMetric,
FCPReportCallback,
MetricRatingThresholds,
ReportCallback,
ReportOpts,
} from './types.js';

Expand Down Expand Up @@ -71,7 +70,7 @@ export const onFCP = (onReport: FCPReportCallback, opts?: ReportOpts) => {

if (po) {
report = bindReporter(
onReport as ReportCallback,
onReport,
metric,
FCPThresholds,
opts!.reportAllChanges
Expand All @@ -82,7 +81,7 @@ export const onFCP = (onReport: FCPReportCallback, opts?: ReportOpts) => {
onBFCacheRestore((event) => {
metric = initMetric('FCP');
report = bindReporter(
onReport as ReportCallback,
onReport,
metric,
FCPThresholds,
opts!.reportAllChanges
Expand Down
7 changes: 3 additions & 4 deletions src/onFID.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import {
FIDReportCallback,
FirstInputPolyfillCallback,
MetricRatingThresholds,
ReportCallback,
ReportOpts,
} from './types.js';

Expand Down Expand Up @@ -71,7 +70,7 @@ export const onFID = (onReport: FIDReportCallback, opts?: ReportOpts) => {

const po = observe('first-input', handleEntries);
report = bindReporter(
onReport as ReportCallback,
onReport,
metric,
FIDThresholds,
opts!.reportAllChanges
Expand Down Expand Up @@ -100,7 +99,7 @@ export const onFID = (onReport: FIDReportCallback, opts?: ReportOpts) => {
onBFCacheRestore(() => {
metric = initMetric('FID');
report = bindReporter(
onReport as ReportCallback,
onReport,
metric,
FIDThresholds,
opts!.reportAllChanges
Expand All @@ -117,7 +116,7 @@ export const onFID = (onReport: FIDReportCallback, opts?: ReportOpts) => {
onBFCacheRestore(() => {
metric = initMetric('FID');
report = bindReporter(
onReport as ReportCallback,
onReport,
metric,
FIDThresholds,
opts!.reportAllChanges
Expand Down
5 changes: 2 additions & 3 deletions src/onINP.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import {
INPMetric,
INPReportCallback,
MetricRatingThresholds,
ReportCallback,
ReportOpts,
} from './types.js';

Expand Down Expand Up @@ -212,7 +211,7 @@ export const onINP = (onReport: INPReportCallback, opts?: ReportOpts) => {
} as PerformanceObserverInit);

report = bindReporter(
onReport as ReportCallback,
onReport,
metric,
INPThresholds,
opts!.reportAllChanges
Expand Down Expand Up @@ -246,7 +245,7 @@ export const onINP = (onReport: INPReportCallback, opts?: ReportOpts) => {

metric = initMetric('INP');
report = bindReporter(
onReport as ReportCallback,
onReport,
metric,
INPThresholds,
opts!.reportAllChanges
Expand Down
5 changes: 2 additions & 3 deletions src/onLCP.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import {
LCPMetric,
MetricRatingThresholds,
LCPReportCallback,
ReportCallback,
ReportOpts,
} from './types.js';

Expand Down Expand Up @@ -82,7 +81,7 @@ export const onLCP = (onReport: LCPReportCallback, opts?: ReportOpts) => {

if (po) {
report = bindReporter(
onReport as ReportCallback,
onReport,
metric,
LCPThresholds,
opts!.reportAllChanges
Expand Down Expand Up @@ -111,7 +110,7 @@ export const onLCP = (onReport: LCPReportCallback, opts?: ReportOpts) => {
onBFCacheRestore((event) => {
metric = initMetric('LCP');
report = bindReporter(
onReport as ReportCallback,
onReport,
metric,
LCPThresholds,
opts!.reportAllChanges
Expand Down
5 changes: 2 additions & 3 deletions src/onTTFB.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import {onBFCacheRestore} from './lib/bfcache.js';
import {getNavigationEntry} from './lib/getNavigationEntry.js';
import {
MetricRatingThresholds,
ReportCallback,
ReportOpts,
TTFBReportCallback,
} from './types.js';
Expand Down Expand Up @@ -66,7 +65,7 @@ export const onTTFB = (onReport: TTFBReportCallback, opts?: ReportOpts) => {

let metric = initMetric('TTFB');
let report = bindReporter(
onReport as ReportCallback,
onReport,
metric,
TTFBThresholds,
opts.reportAllChanges
Expand Down Expand Up @@ -100,7 +99,7 @@ export const onTTFB = (onReport: TTFBReportCallback, opts?: ReportOpts) => {
onBFCacheRestore(() => {
metric = initMetric('TTFB', 0);
report = bindReporter(
onReport as ReportCallback,
onReport,
metric,
TTFBThresholds,
opts!.reportAllChanges
Expand Down
17 changes: 16 additions & 1 deletion src/types/base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@ import {
FirstInputPolyfillEntry,
NavigationTimingPolyfillEntry,
} from './polyfills.js';
import type {CLSMetric} from './cls.js';
import type {FCPMetric} from './fcp.js';
import type {FIDMetric} from './fid.js';
import type {INPMetric} from './inp.js';
import type {LCPMetric} from './lcp.js';
import type {TTFBMetric} from './ttfb.js';
Copy link
Member Author

Choose a reason for hiding this comment

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

import type vs import here doesn't change anything in the output since these files (currently) have only types in them, but generally good to be explicit that the circular dependency is purely in type-space and so won't ever be an issue


export interface Metric {
/**
Expand Down Expand Up @@ -85,6 +91,15 @@ export interface Metric {
| 'restore';
}

/** The union of supported metric types. */
export type MetricType =
| CLSMetric
| FCPMetric
| FIDMetric
| INPMetric
| LCPMetric
| TTFBMetric;

/**
* A version of the `Metric` that is used with the attribution build.
*/
Expand Down Expand Up @@ -113,7 +128,7 @@ export interface MetricWithAttribution extends Metric {
export type MetricRatingThresholds = [number, number];

export interface ReportCallback {
(metric: Metric): void;
(metric: MetricType): void;
}

export interface ReportOpts {
Expand Down
2 changes: 1 addition & 1 deletion src/types/cls.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import {LoadState, Metric} from './base.js';
import type {LoadState, Metric} from './base.js';

/**
* A CLS-specific version of the Metric object.
Expand Down
2 changes: 1 addition & 1 deletion src/types/fcp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import {LoadState, Metric} from './base.js';
import type {LoadState, Metric} from './base.js';
import {NavigationTimingPolyfillEntry} from './polyfills.js';

/**
Expand Down
2 changes: 1 addition & 1 deletion src/types/fid.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import {LoadState, Metric} from './base.js';
import type {LoadState, Metric} from './base.js';
import {FirstInputPolyfillEntry} from './polyfills.js';

/**
Expand Down
2 changes: 1 addition & 1 deletion src/types/inp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import {LoadState, Metric} from './base.js';
import type {LoadState, Metric} from './base.js';

/**
* An INP-specific version of the Metric object.
Expand Down
2 changes: 1 addition & 1 deletion src/types/lcp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import {Metric} from './base.js';
import type {Metric} from './base.js';
import {NavigationTimingPolyfillEntry} from './polyfills.js';

/**
Expand Down
2 changes: 1 addition & 1 deletion src/types/ttfb.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import {Metric} from './base.js';
import type {Metric} from './base.js';
import {NavigationTimingPolyfillEntry} from './polyfills.js';

/**
Expand Down