Rename session.navigator during destructuring to avoid clobbering window.navigator
#1489
Conversation
…dow.navigator When Rollup's output format changed to "esm"[1], it stopped automatically renaming variables that shadow globals. This caused the bundled turbo.js to overwrite `window.navigator` with Turbo's internal `Navigator` instance, breaking libraries that depend on the browser's `navigator`. Fix by explicitly renaming `navigator` to `sessionNavigator` during destructuring, then re-exporting it under the original name. Fixes hotwired/turbo-rails#776 [1]: cbf3d86
| // Rename `navigator` to avoid shadowing `window.navigator` | ||
| const { cache, navigator: sessionNavigator } = session | ||
| export { session, cache, sessionNavigator as navigator } |
There was a problem hiding this comment.
Depending on how we've been supporting this particular property, this could be considered a breaking change. For example, there is existing test coverage that ensures that Turbo.navigator is available when imported with a *.
turbo/src/tests/unit/export_tests.js
Line 19 in 9f5b6c4
I think a more backwards-compatible resolution could involve rolling back the Rollup config to its previous value, followed up by a more deliberate deprecation and rename of this export.
What do you think?
There was a problem hiding this comment.
Ahh, I see that the final export is still consistently navigator, so that's good. Maybe this could work?
There was a problem hiding this comment.
Yes, exactly. It was just the destructured navigator constant that was clobbering the global window object of the same name. We still export as Turbo.navigator.
When Rollup's output format changed to "esm" in #1016, it stopped automatically renaming variables that shadow globals. This caused the bundled turbo.js to overwrite
window.navigatorwith Turbo's internalNavigatorinstance, breaking libraries that depend on the browser'swindow.navigator.Fix by renaming
navigatortosessionNavigatorduring destructuring, then re-exporting it under the original name.Fixes hotwired/turbo-rails#776