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

performance now tracks first-input-delay #542

Merged
merged 1 commit into from
Mar 1, 2019

Conversation

GoodForOneFare
Copy link
Member

Basically, this measures the milliseconds between the user's first event, and the browser passing the event on to handlers. See https://github.com/GoogleChromeLabs/first-input-delay for more information.

Note: this is only useful when the first-input-delay polyfill is present in the document's head.

@GoodForOneFare GoodForOneFare force-pushed the performance-listen-for-first-input-delay branch from cf64073 to cdf859b Compare February 28, 2019 20:22
@GoodForOneFare
Copy link
Member Author

Trying to figure out tests for this, but I'd appreciate comments now :)


if (typeof perfMetrics !== 'undefined') {
perfMetrics.onFirstInputDelay(delay => {
this.lifecycleEvent({
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this really a lifecycle event? It's a thing that happens once, so I think so. But it's also the only one that concerns duration instead of start. Should we expose a way for consumers to know which value is significant?

Copy link
Member

Choose a reason for hiding this comment

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

This definitely does feel a bit weird but I think we are kind of stuck putting it here. Consumers at least have a way of figuring out whether it was some sort of duration event or a point in time by looking at duration, but I agree it's not the cleanest abstraction. IMO, this is fine for the time being.

Copy link
Member

@lemonmade lemonmade left a comment

Choose a reason for hiding this comment

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

Code looks very good 👍

packages/performance/src/performance.ts Outdated Show resolved Hide resolved
packages/performance/src/first-input-delay.d.ts Outdated Show resolved Hide resolved

if (typeof perfMetrics !== 'undefined') {
perfMetrics.onFirstInputDelay(delay => {
this.lifecycleEvent({
Copy link
Member

Choose a reason for hiding this comment

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

This definitely does feel a bit weird but I think we are kind of stuck putting it here. Consumers at least have a way of figuring out whether it was some sort of duration event or a point in time by looking at duration, but I agree it's not the cleanest abstraction. IMO, this is fine for the time being.

Copy link
Contributor

@marutypes marutypes left a comment

Choose a reason for hiding this comment

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

Code looks good, but we should add changelog, tests

@GoodForOneFare GoodForOneFare force-pushed the performance-listen-for-first-input-delay branch from 702ba1f to 15cd659 Compare March 1, 2019 05:05
@@ -12,4 +16,47 @@ describe('Performance', () => {
performance.finish();
expect(spy).toHaveBeenCalledWith(expect.any(Navigation));
});

describe.skip('first-input-delay', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

@lemonmade tests aren't runnable right now, but I think they'd pass once a performance mock is available. Can you sanity check, please?

Copy link
Member

@lemonmade lemonmade left a comment

Choose a reason for hiding this comment

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

Tests make a lot of sense 👍 Probably worth adding a comment above them to say what is needed to remove the skip

performance.on('lifecycleEvent', lifecycleSpy);

// eslint-disable-next-line typescript/no-non-null-assertion
perfMetrics.callback!(1000, new Event('click'));
Copy link
Member

Choose a reason for hiding this comment

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

Oh interesting, I wonder if we should capture the event type as metadata for the fid event?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's get some results in first (aka, I'd like to ship this now 😛 )

@GoodForOneFare GoodForOneFare force-pushed the performance-listen-for-first-input-delay branch from 87916e0 to 6b49b1f Compare March 1, 2019 20:24
Basically, this measures the milliseconds between the user's first event, and the browser passing the event on to handlers.  See https://github.com/GoogleChromeLabs/first-input-delay for more information.

Note: this is only useful when the first-input-delay polyfill is present in the document's head.
@GoodForOneFare GoodForOneFare force-pushed the performance-listen-for-first-input-delay branch from 6b49b1f to 88c1467 Compare March 1, 2019 20:26
@GoodForOneFare GoodForOneFare merged commit 91e33cd into master Mar 1, 2019
@GoodForOneFare GoodForOneFare deleted the performance-listen-for-first-input-delay branch March 1, 2019 20:44
@lemonmade lemonmade temporarily deployed to production March 2, 2019 16:13 Inactive
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.

3 participants