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

Avoid infinite recursion from window.fetch name collision #1077

Merged
merged 1 commit into from
Nov 27, 2023

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented Nov 27, 2023

Closes @hotwired/turbo-rails#526

Prevent naming collisions between the new Turbo.fetch function and the built-in window.fetch function.

To do so, create a module-local reference to window.fetch, and define the exported function as fetchWithTurboHeaders. At export-time, rename to fetch so that consumers can continue to import it as Turbo.fetch.

Closes [hotwired#526][]

Prevent naming collisions between the new `Turbo.fetch` function and the
built-in `window.fetch` function.

To do so, create a module-local reference to `window.fetch`, and define
the exported function as `fetchWithTurboHeaders`. At export-time, rename
to `fetch` so that consumers can continue to import it as `Turbo.fetch`.

[hotwired#526]: hotwired/turbo-rails#526
@seanpdoyle
Copy link
Contributor Author

seanpdoyle commented Nov 27, 2023

@jorgemanrubia @afcapel I'm not entirely sure why this resolves the issue.

During my investigation, I found that the build output was using var in a way that was defining any exported fetch as a property on the window.. First with the Turbo-specific window.fetch override, then with the fetch method that Stimulus' MultiMap implementation ships.

I think something is either wrong built the build tooling configuration, or something else at the browser layer.

Copy link
Member

@jorgemanrubia jorgemanrubia left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this one @seanpdoyle

@jorgemanrubia jorgemanrubia merged commit 325216e into hotwired:main Nov 27, 2023
1 check passed
@seanpdoyle
Copy link
Contributor Author

What can we do to fix the underlying issue?

How can we prevent future changes from reintroducing the regression?

The test suite did not catch it.

@jorgemanrubia
Copy link
Member

Something weird is that turbo-rails uses its own bundling pipeline, which is not the same that Turbo uses, and I suspect that is a factor here. I'm not sure about the historical reasons for that, but we should unify those.

@afcapel
Copy link
Collaborator

afcapel commented Nov 27, 2023

The problem here is that a new rails new demo -a propshaft -T -j bun -c tailwind is adding <%= javascript_include_tag "application", "data-turbo-track": "reload", defer: true %> to application.html.erb. The default type for javascript_include_tag is "text/javascript", which is incorrect in this case. javascript_include_tag is pointing to ES6 code that imports other modules, the correct type should be "module". Otherwise all the variables inside the ESM module are incorrectly assigned in the global scope.

I checked that adding type: "module" to the javascript_include_tag call fixes the issue.

This doesn't happen using importmaps because importmap-rails adds <script type="module">import "application"</script> to the application layout instead.

@seanpdoyle seanpdoyle deleted the avoid-fetch-name-collisions branch November 27, 2023 15:24
@afcapel
Copy link
Collaborator

afcapel commented Nov 27, 2023

I can reproduce this with Turbo 7.3 and bun, same pollution of the global window scope. I think we only noticed now because there was a collision between window.fetch and Turbo.fetch.

afcapel added a commit to afcapel/jsbundling-rails that referenced this pull request Nov 27, 2023
Bun.js generates JS bundles in the ESM format and they need be imported with
the `type="module"` attribute. Otherwise the module varibles end up in the
global scope. See hotwired/turbo#1077

This commit updates the install generator to add the type="module" attribute
to the default `javascript_include_tag`.

`defer` is no longer needed, as JS modules are deferred by default.

Ref. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules#other_differences_between_modules_and_standard_scripts

This PR also updates the default config to ensure that all bundlers are
configured to output ESM bundles.

- bun only supports ESM at the moment https://bun.sh/docs/bundler#format
- esbuild outputs ESM by default when bundling is enabled https://esbuild.github.io/api/#format-esm
- webpacker is configured to output ESM bundles with `output.chunkFormat` https://webpack.js.org/configuration/output/#outputchunkformat
- rollup is configured to output ESM bundles with `output.format` https://rollupjs.org/configuration-options/#output-format
@afcapel
Copy link
Collaborator

afcapel commented Nov 27, 2023

Fix in js-bundling-rails here rails/jsbundling-rails#178

@jorgemanrubia
Copy link
Member

Amazing work tracking this one down @afcapel!

@seanpdoyle
Copy link
Contributor Author

Would it be worth reverting this commit, since the fix is better suited elsewhere?

@jorgemanrubia
Copy link
Member

I think the change is ok @seanpdoyle. It uses a clearer naming approach IMO.

afcapel added a commit to afcapel/jsbundling-rails that referenced this pull request Nov 29, 2023
Bun.js generates JS bundles in the ESM format and they need be imported with
the `type="module"` attribute. Otherwise the module varibles end up in the
global scope. See hotwired/turbo#1077

This commit updates the install generator to add the type="module" attribute
to the default `javascript_include_tag`.

`defer` is no longer needed, as JS modules are deferred by default.

Ref. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules#other_differences_between_modules_and_standard_scripts

This PR also updates the default config to ensure that all bundlers are
configured to output ESM bundles.

- bun only supports ESM at the moment https://bun.sh/docs/bundler#format
- esbuild is configured to output ESM with the --format=esm flag https://esbuild.github.io/api/#format-esm
- webpacker is configured to output ESM bundles with `output.chunkFormat` https://webpack.js.org/configuration/output/#outputchunkformat
- rollup is configured to output ESM bundles with `output.format` https://rollupjs.org/configuration-options/#output-format
@KonnorRogers
Copy link
Contributor

I can reproduce this with Turbo 7.3 and bun, same pollution of the global window scope. I think we only noticed now because there was a collision between window.fetch and Turbo.fetch.

+1. This behavior still exists on @hotwired/turbo@8.beta-1 using Bun which comes with Rails new. Not sure if a new release has been made.

dhh pushed a commit to rails/jsbundling-rails that referenced this pull request Jan 5, 2024
Bun.js generates JS bundles in the ESM format and they need be imported with
the `type="module"` attribute. Otherwise the module varibles end up in the
global scope. See hotwired/turbo#1077

This commit updates the install generator to add the type="module" attribute
to the default `javascript_include_tag`.

`defer` is no longer needed, as JS modules are deferred by default.

Ref. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules#other_differences_between_modules_and_standard_scripts

This PR also updates the default config to ensure that all bundlers are
configured to output ESM bundles.

- bun only supports ESM at the moment https://bun.sh/docs/bundler#format
- esbuild is configured to output ESM with the --format=esm flag https://esbuild.github.io/api/#format-esm
- webpacker is configured to output ESM bundles with `output.chunkFormat` https://webpack.js.org/configuration/output/#outputchunkformat
- rollup is configured to output ESM bundles with `output.format` https://rollupjs.org/configuration-options/#output-format
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

RangeError: Maximum call stack size exceeded
4 participants