inertiajs / inertia

Inertia.js lets you quickly build modern single-page React, Vue and Svelte apps using classic server-side routing and controllers.
https://inertiajs.com
MIT License
6.3k stars 423 forks source link

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

Closed hivokas closed 1 week ago

hivokas commented 1 month ago

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.

reinink commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

@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: https://github.com/inertiajs/inertia/blob/338b4c25a68ff0a62c1fedf96789d0ff4273f21c/packages/core/src/navigationType.ts#L5-L9

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 commented 3 weeks ago

@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:

https://github.com/inertiajs/inertia/blob/338b4c25a68ff0a62c1fedf96789d0ff4273f21c/packages/core/src/navigationType.ts#L5-L9

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

hivokas commented 1 week ago

Thanks!