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

Improve handling around performance navigation entry #74

Merged
merged 1 commit into from
Jun 6, 2023

Conversation

hongrich
Copy link
Contributor

@hongrich hongrich commented Jun 6, 2023

Older browsers do not have support for PerformanceNavigationTiming. This changes adds fallback for getting navigation type from the deprecated window.performance.navigation.type when PerformanceNavigationTiming is not available.

Since activationStart is also on PerformanceNavigationTiming, we move this into a helper in the same file as well.

Finally, we can override the built-in browser declarations to get better typing for getEntriesByType, activationStart, and document.prerendering. This allows us to remove the override for @typescript-eslint/ban-ts-comment eslint rule since the default is pretty reasonable (ts-expect-error is allowed with description) https://typescript-eslint.io/rules/ban-ts-comment/

@hongrich hongrich force-pushed the hongrich/navigation-entry branch 2 times, most recently from e6ab879 to 14dc245 Compare June 6, 2023 17:52
@hongrich hongrich marked this pull request as ready for review June 6, 2023 18:03
Copy link
Member

@ajhyndman ajhyndman left a comment

Choose a reason for hiding this comment

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

This looks sensible to me!

Nit: If you feel like sprinkling some assertions about navigationType around our existing test cases (especially spa test cases) that would be cool to include.

src/navigationEntry.ts Outdated Show resolved Hide resolved
src/navigationEntry.ts Outdated Show resolved Hide resolved
@hongrich hongrich force-pushed the hongrich/navigation-entry branch 2 times, most recently from 6da4c18 to 75ee7f5 Compare June 6, 2023 21:43
Older browsers do not have support for PerformanceNavigationTiming. This changes adds fallback for getting navigation type from the deprecated `window.performance.navigation.type` when PerformanceNavigationTiming is not available.

Since `activationStart` is also on PerformanceNavigationTiming, we move this into a helper in the same file as well.

Finally, we can override the built-in browser declarations to get better typing for `getEntriesByType`, `activationStart`, and `document.prerendering`. This allows us to remove the override for `@typescript-eslint/ban-ts-comment` eslint rule since the default is pretty reasonable (`ts-expect-error` is allowed with description) https://typescript-eslint.io/rules/ban-ts-comment/
Copy link
Member

@ajhyndman ajhyndman left a comment

Choose a reason for hiding this comment

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

LGTM!

@hongrich hongrich merged commit 237266c into main Jun 6, 2023
@hongrich hongrich deleted the hongrich/navigation-entry branch June 6, 2023 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants