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

Add guard against getEntriesByType #553

Closed
wants to merge 2 commits into from
Closed

Add guard against getEntriesByType #553

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bc5db2c
Add guard against getEntriesByType
tunetheweb Oct 22, 2024
16f4a51
Review feedback
tunetheweb Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/lib/getNavigationEntry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
*/

export const getNavigationEntry = (): PerformanceNavigationTiming | void => {
const navigationEntry = performance.getEntriesByType('navigation')[0];
// JSDOM does not implement getEntriesByType: https://github.com/jsdom/jsdom/issues/3309
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels sad to accommodate an inadequacy of a test-env-specific library, given that this API is supported in all major browsers: https://caniuse.com/mdn-api_performance_getentriesbytype

Copy link
Member Author

@tunetheweb tunetheweb Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree. However it's a guard, rather than a polyfill, or holding ourselves back in using new features or syntax. So I'm comfortable with this. We've a few cases of this in the library.

But let's see if @philipwalton agrees!

Copy link
Member

Choose a reason for hiding this comment

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

Optional chaining could make this not so bad, but in general if you're using jsdom for testing, you'll be mocking out a bunch of stuff anyways and it shouldn't generally be put on the library to do that.

Nothing will work in jsdom anyways, looks like there's no PerformanceObservers either, so I wonder if the tester should just short circuit it completely.

Copy link
Member Author

Choose a reason for hiding this comment

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

For those we wrap it in a try/catch anyway:
https://github.com/GoogleChrome/web-vitals/blob/v5/src/lib/observe.ts
Hence why they didn’t cause a problem.

For TTFB however we just call it as it doesn’t need a PerformanceObserver.

I don’t think the intent is to test the library in JSDOM, it’s more the whole page is being loaded in that (including this script) and it hits this error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh and I tried optional chaining initially and still complained getEntriesByTyoe wasn’t a function. Because performance does exist but not much under it. Open to ideas on how to implement this better if you know some better optional chaining syntax for that? For now I just reverted back to what we had in v4.

Copy link
Member

Choose a reason for hiding this comment

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

I don’t think the intent is to test the library in JSDOM, it’s more the whole page is being loaded in that (including this script) and it hits this error.

right, but I guarantee this isn't the only thing broken in jsdom and they already have a "set up the mocks" section somewhere because there's always something (usually several things) that need to be mocked out

Open to ideas on how to implement this better if you know some better optional chaining syntax for that?

performance.getEntriesByType?.('navigation')[0] should work

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh that worked! Thank you. Updated in 16f4a51

const navigationEntry = performance.getEntriesByType?.('navigation')[0];

// Check to ensure the `responseStart` property is present and valid.
// In some cases a zero value is reported by the browser (for
Expand Down