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.
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
Make bindReporter
generic over metric type
#359
Make bindReporter
generic over metric type
#359
Conversation
@@ -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[]) => { |
There was a problem hiding this comment.
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 :)
@@ -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[] = []; |
There was a problem hiding this comment.
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
import type {FIDMetric} from './fid.js'; | ||
import type {INPMetric} from './inp.js'; | ||
import type {LCPMetric} from './lcp.js'; | ||
import type {TTFBMetric} from './ttfb.js'; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but with one question.
581705c
to
591e059
Compare
bindReporter
generic over metric type
Can land after #356
Parameterizes the
bindReporter
type on themetric
passed in so that thecallback
can now be typed specifically for that metric (e.g. a result forCLSMetric
must be using aCLSReportCallback
).This required
initMetric
to be parameterized similarly, so the returned metric type is now based on the metricname
passed in.The main change required was using a union of the metric types wherever the base
Metric
interface is currently used. This leaves most uses of it unchanged, but allows narrowing to which specific metric is being used based on thename
property.I renamed the existing
Metric
to beMetricBase
and named the unionMetric
to keep the uses of it unchanged, but if someone has better naming suggestions, changing is no problem :)