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

[1.x] Fix setNavigationType for Safari 10 #1957

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

hivokas
Copy link
Contributor

@hivokas hivokas commented Sep 2, 2024

Simple fix for Safari 10.

Screenshot 2024-09-02 at 2 02 34 PM

It happens because this.navigation is present, but this.navigation.getEntriesByType is missing.

Copy link
Contributor

@derrickreimer derrickreimer left a comment

Choose a reason for hiding this comment

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

It looks like Safari was one of the latest to add support for this (in 2017). https://developer.mozilla.org/en-US/docs/Web/API/Performance/getEntriesByType#browser_compatibility

Arguably this affects an extremely small % of users, but I don't really see a drawback in adding one additional guard to this ternary.

@reinink
Copy link
Member

reinink commented Sep 2, 2024

@hivokas Hey thanks for this! We've actually already corrected this in our v2.0 branch (see here), but I think it would be smart to fix this in 1.x as well 👍

@hivokas
Copy link
Contributor Author

hivokas commented Sep 2, 2024

Arguably this affects an extremely small % of users, but I don't really see a drawback in adding one additional guard to this ternary.

@derrickreimer Yeah, it's indeed an extremely small % of users, but useful for projects which aim to support a wide variety of browsers.

@hivokas
Copy link
Contributor Author

hivokas commented Sep 2, 2024

@hivokas Hey thanks for this! We've actually already corrected this in our v2.0 branch (see here), but I think it would be smart to fix this in 1.x as well 👍

@reinink Welcome!

By the way, I've checked the code from the link you provided:

if (window?.performance.getEntriesByType('navigation').length > 0) {
this.type = (window.performance.getEntriesByType('navigation')[0] as PerformanceNavigationTiming).type
} else {
this.type = 'navigate'
}

Unless I'm missing something, I don't think window?.performance.getEntriesByType handles this Safari 10 edge case. Should it be window?.performance?.getEntriesByType instead? (because Safari has windows.performance, but doesn't have window.performance.getEntriesByType).

@jamesst20
Copy link
Contributor

@hivokas Hey thanks for this! We've actually already corrected this in our v2.0 branch (see here), but I think it would be smart to fix this in 1.x as well 👍

@reinink Welcome!

By the way, I've checked the code from the link you provided:

if (window?.performance.getEntriesByType('navigation').length > 0) {
this.type = (window.performance.getEntriesByType('navigation')[0] as PerformanceNavigationTiming).type
} else {
this.type = 'navigate'
}

Unless I'm missing something, I don't think window?.performance.getEntriesByType handles this Safari 10 edge case. Should it be window?.performance?.getEntriesByType instead? (because Safari has windows.performance, but doesn't have window.performance.getEntriesByType).

Then in this case both will be wrong as you can't call a function if it is undefined even with the ? operator on the previous assignment.

Proper signature would be

window.performance.getEntriesByType?.(...)

Screenshot 2024-09-09 at 11 37 59 AM

@pedroborges pedroborges changed the title Fix setNavigationType for Safari 10 [1.x] Fix setNavigationType for Safari 10 Sep 13, 2024
Copy link
Collaborator

@pedroborges pedroborges left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks @hivokas!

@pedroborges pedroborges added the core Related to the core Inertia library label Sep 23, 2024
Copy link
Contributor

@joetannenbaum joetannenbaum left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@pedroborges pedroborges merged commit 5af4ab9 into inertiajs:master Sep 25, 2024
5 checks passed
@hivokas
Copy link
Contributor Author

hivokas commented Sep 25, 2024

Thanks!

@hivokas
Copy link
Contributor Author

hivokas commented Dec 22, 2024

@joetannenbaum even though it works in 1.x, it doesn't seem to work in 2.x. Is there a chance this fix could be ported to 2.x?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Related to the core Inertia library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants