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.

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

Finish adding getINP() #221

Merged
merged 2 commits into from Apr 25, 2022
Merged

Finish adding getINP() #221

merged 2 commits into from Apr 25, 2022

Conversation

philipwalton
Copy link
Member

Fixes #208.

This PR builds on top of what @mmocny added in #213 with the following:

  • Updates the observe module logic to support processing a list of entries rather than each individually.
  • Moves the interactionCount logic into its own module and rewrites it as a "polyfill" so it won't run if the browser natively supports interactionCount (hopefully coming to Chrome soon).
  • Sets the Metric.entries value to all reported entries from the INP interaction, rather than just the longest. (As discussed we should extend this to all entries in the frame, but we probably need a dedicated API for this first.)
  • Updates the durationThreshold to 50 as a compromise between performance and more accurate duration values at lower percentiles.
  • Adds bfcache support.
  • Adds tests

Copy link
Member

@mmocny mmocny left a comment

Choose a reason for hiding this comment

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

Changes look solid, nice!

Most comments are nits, but I think a few may be actual bugs.

I didn't review or try the unit tests.

src/getINP.ts Outdated Show resolved Hide resolved
src/getINP.ts Outdated
if (existingInteractionIndex >= 0) {
const interaction = longestInteractions[existingInteractionIndex];
interaction.latency = Math.max(interaction.latency, entry.duration);
interaction.entries.push(entry);
Copy link
Member

Choose a reason for hiding this comment

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

Will these be sorted by duration ever?

I think we have sample code that uses .entries[0] and performance.measure, as a simple way to visualize the latency on timeline, for example.

Copy link
Member Author

Choose a reason for hiding this comment

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

That was an open question I had (what to sort them by). I think (?) we want to keep them in the order they were dispatched and leave any sorting up to the debug code, but let me know if you disagree and think it's important to have the largest duration first.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, fair enough.

While I think making the first entry the largest would be reasonable given the current implementation -- my ultimate preference is to report all entries that present the same frame such that we can create a summary of where time was spent overall. (Or even just report the summary as part of the Metric return value).

So leaving it up to debug code sgtm.

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 very much agree with that ultimate goal. Let's keep it simple for now and re-evaluate once we have more options there or when we plan to release this as stable.

src/getINP.ts Show resolved Hide resolved
}
const handleEntries = (entries: Metric['entries']) => {
(entries as PerformanceEventTiming[]).forEach((entry) => {
if (entry.interactionId) {
Copy link
Member

Choose a reason for hiding this comment

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

FYI, we could potentially polyfill for Firefox by assigning an interactionId for the entry here, for events of specifics types. This would include more events than we want, ideally, but could still be useful.

interactionCount estimate would be off, so we could just always return 1.

I'm not sure how much error that approach would have, and how that compares to historical attempts to polyfill features.

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 can experiment.

I did experience some with your earlier polyfill using eventCounts, and that didn't work in Firefox because (IIRC) it doesn't seem to emit pointercancel events. I could try this and see how well it works.

src/getINP.ts Outdated Show resolved Hide resolved
src/getINP.ts Outdated Show resolved Hide resolved
src/getINP.ts Show resolved Hide resolved
src/getINP.ts Outdated Show resolved Hide resolved
src/lib/polyfills/interactionCountPolyfill.ts Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants