fix: Record resource timing after load event #450
Conversation
qhanam
force-pushed
the
fix-resources-after-load
branch
from
d6e2972
to
6fe2f26
Compare
| @@ -132,6 +114,12 @@ export class ResourcePlugin extends InternalPlugin { | |||
| }; | |||
|
|
|||
| protected onload(): void { | |||
| window.addEventListener(LOAD, this.resourceEventListener); | |||
| // We need to set `buffered: true`, so the observer also records past | |||
There was a problem hiding this comment.
Question: are will still concerned about using performance.geEntries() as backup for browsers that do not support PerformanceObserver?
There was a problem hiding this comment.
| } | ||
| }); | ||
| } | ||
| list.filter((e) => e.entryType === RESOURCE) |
There was a problem hiding this comment.
question: aren't these already filtered?
performanceObserver.observe({type: 'resource'...})
| ); | ||
|
|
||
| await t.expect(events.length).eql(1); | ||
| }); | ||
|
|
||
| test('when resource loads after window.load then the resource is recorded', async (t: TestController) => { | ||
| test('when resource loads before the plugin then the resource is recorded', async (t: TestController) => { |
There was a problem hiding this comment.
Question: When does window.load occur during TestCafe test? Why did reversing these fix the problem?
There was a problem hiding this comment.
Firefox seems caching the image between runs, so during the second run it doesn't make a request to the server for it.
I assume there is some Firefox (or TestCafe) configuration that prevents this caching.
There was a problem hiding this comment.
Switched the tests in #448 like you did here
|
I agree with the implementation. Only blocking thing for me to understand how you fixed that resource plugin integration test |
| ] | ||
| ResourceType.FONT | ||
| ], | ||
| sampleTypes: [ResourceType.IMAGE, ResourceType.OTHER] |
There was a problem hiding this comment.
Question: Why are images not included? This may impact lcp attribution. I remember you considered recording all types
| list.filter((e) => e.entryType === RESOURCE) | ||
| .filter((e) => !this.config.ignore(e)) | ||
| .forEach((event) => { | ||
| const type: ResourceType = getResourceFileType(event.name); |
There was a problem hiding this comment.
Issue: I have found that sometimes event.name does not include the correct file type extension. This can cause getResourceFileType to fail. Could we add logic to use initatorType as a backup method to verify this information?
There was a problem hiding this comment.
I agree using initiatorType sounds like a good idea to get more accurate resource types. That is, instead of using only the file extension to identify the resource type,getResourceFileType would also use initiatorType.
I believe this is a new feature though. In #448, could you also use getResourceFileType? Then we can make a separate PR to improve getResourceFileType accuracy by considering initatorType.
The web client does not currently record PerformanceResource entries after the load event fires. This means that slow, asynchronous resources, that contribute to poor web vitals like LCP or CLS, will not be recorded.
This change modifies the PerformanceResource plugin to continue listening to PerformanceResource entries after the load event.
To accomplish this, we modify our resource prioritization in the following ways:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.