Skip to content

Commit f761780

Browse files
Fix rails-ujs auto start() in bundled environments
An issue was recently raised that users were seeing an error when upgrading `@rails/ujs` to 7.1.0+: ``` Uncaught Error: rails-ujs has already been loaded! ``` Generally this issue appears due to the difference in how rails-ujs is expected to behave in a pure sprockets environment (js appending) vs a bundled environment (webpack, esbuild, etc.). The sprockets environment is supposed to call start() automatically, while the bundled environment is supposed to require users to import and call start() themselves. As part of the transition from coffeescript to javascript, a condition was added that was intended to detect whether the current environment was a bundler so that it would only call start() when in sprockets. However, this condition is not working as expected, and I was able to reproduce the error appearing when using rails-ujs with importmaps and esbuild. Interestingly, the issue did not appear when using Webpack as a bundler. I believe the best fix here is to make the condition very explicit. Since sprockets users should not be using the esm version of rails-ujs, we can use the rollup replace plugin to explicitly opt the esm bundle out of _ever_ calling start() automatically. This works because terser will run after the replace plugin and remove the whole condition as dead code (since it sees true == false). Co-authored-by: Ryunosuke Sato <[email protected]>
1 parent 633eb5a commit f761780

File tree

8 files changed

+61
-12
lines changed

8 files changed

+61
-12
lines changed

actionview/.eslintrc

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
{
22
"extends": "eslint:recommended",
3+
"globals": {
4+
"__esm": "readonly"
5+
},
36
"rules": {
47
"semi": ["error", "never"],
58
"quotes": ["error", "double"],

actionview/CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
* Fix `@rails/ujs` calling `start()` an extra time when using bundlers
2+
3+
*Hartley McGuire*, *Ryunosuke Sato*
4+
15
* Fix the `capture` view helper compatibility with HAML and Slim
26

37
When a blank string was captured in HAML or Slim (and possibly other template engines)

actionview/app/assets/javascripts/rails-ujs.esm.js

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -683,11 +683,4 @@ if (typeof jQuery !== "undefined" && jQuery && jQuery.ajax) {
683683
}));
684684
}
685685

686-
if (typeof exports !== "object" && typeof module === "undefined") {
687-
window.Rails = Rails;
688-
if (fire(document, "rails:attachBindings")) {
689-
start();
690-
}
691-
}
692-
693686
export { Rails as default };

actionview/app/javascript/rails-ujs/index.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@ if (typeof jQuery !== "undefined" && jQuery && jQuery.ajax) {
149149
// difference between what happens in a bundler and what happens using a
150150
// sprockets compiler. In the sprockets case, Rails.start() is called
151151
// automatically, but it is not in the ESModule case.
152-
if (typeof exports !== "object" && typeof module === "undefined") {
152+
if (__esm == false && typeof exports !== "object" && typeof module === "undefined") {
153153
// The coffeescript bundle would set this at the very top. The Rollup bundle
154154
// doesn't set this until the entire bundle has finished running, so we need
155155
// to make sure its set before firing the rails:attachBindings event for

actionview/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
"devDependencies": {
3535
"@rollup/plugin-commonjs": "^19.0.1",
3636
"@rollup/plugin-node-resolve": "^11.0.1",
37+
"@rollup/plugin-replace": "^5.0.4",
3738
"eslint": "^4.19.1",
3839
"eslint-plugin-import": "^2.23.4",
3940
"jquery": "^2.2.0",

actionview/rollup.config.js

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { terser } from "rollup-plugin-terser"
2+
import replace from "@rollup/plugin-replace"
23

34
const banner = `
45
/*
@@ -33,7 +34,11 @@ export default [
3334
banner,
3435
},
3536
plugins: [
36-
terser(terserOptions)
37+
replace({
38+
preventAssignment: true,
39+
values: { __esm: false },
40+
}),
41+
terser(terserOptions),
3742
]
3843
},
3944

@@ -45,7 +50,11 @@ export default [
4550
banner,
4651
},
4752
plugins: [
48-
terser(terserOptions)
53+
replace({
54+
preventAssignment: true,
55+
values: { __esm: true },
56+
}),
57+
terser(terserOptions),
4958
]
5059
}
5160
]

actionview/rollup.config.test.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Rollup configuration for compiling the UJS tests
22

33
import commonjs from "@rollup/plugin-commonjs"
4+
import replace from "@rollup/plugin-replace"
45
import resolve from "@rollup/plugin-node-resolve"
56

67
export default {
@@ -12,6 +13,10 @@ export default {
1213
},
1314

1415
plugins: [
16+
replace({
17+
preventAssignment: true,
18+
values: { __esm: false }, // false because the tests expects start() to be called automatically
19+
}),
1520
resolve(),
1621
commonjs()
1722
]

yarn.lock

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,11 @@
2828
resolved "https://registry.npmjs.org/@jridgewell/source-map/-/source-map-0.3.4.tgz"
2929
integrity sha512-KE/SxsDqNs3rrWwFHcRh15ZLVFrI0YoZtgAdIyIq9k5hUNmiWRXXThPomIxHuL20sLdgzbDFyvkUMna14bvtrw==
3030

31+
"@jridgewell/sourcemap-codec@^1.4.15":
32+
version "1.4.15"
33+
resolved "https://registry.npmjs.org/@jridgewell/sourcemap-codec/-/sourcemap-codec-1.4.15.tgz"
34+
integrity sha512-eF2rxCRulEKXHTRiDrDy6erMYWqNw4LPdQ8UQA4huuxaQsVeRPFl2oM8oDGxMFhJUWZf9McpLtJasDDZb/Bpeg==
35+
3136
"@rails/activestorage@>= 7.1.0-alpha":
3237
version "7.1.0-beta1"
3338
resolved "https://registry.npmjs.org/@rails/activestorage/-/activestorage-7.1.0-beta1.tgz"
@@ -60,6 +65,14 @@
6065
is-module "^1.0.0"
6166
resolve "^1.19.0"
6267

68+
"@rollup/plugin-replace@^5.0.4":
69+
version "5.0.4"
70+
resolved "https://registry.yarnpkg.com/@rollup/plugin-replace/-/plugin-replace-5.0.4.tgz#fef548dc751d06747e8dca5b0e8e1fbf647ac7e1"
71+
integrity sha512-E2hmRnlh09K8HGT0rOnnri9OTh+BILGr7NVJGB30S4E3cLRn3J0xjdiyOZ74adPs4NiAMgrjUMGAZNJDBgsdmQ==
72+
dependencies:
73+
"@rollup/pluginutils" "^5.0.1"
74+
magic-string "^0.30.3"
75+
6376
"@rollup/pluginutils@^3.1.0":
6477
version "3.1.0"
6578
resolved "https://registry.npmjs.org/@rollup/pluginutils/-/pluginutils-3.1.0.tgz"
@@ -69,11 +82,25 @@
6982
estree-walker "^1.0.1"
7083
picomatch "^2.2.2"
7184

85+
"@rollup/pluginutils@^5.0.1":
86+
version "5.0.5"
87+
resolved "https://registry.yarnpkg.com/@rollup/pluginutils/-/pluginutils-5.0.5.tgz#bbb4c175e19ebfeeb8c132c2eea0ecb89941a66c"
88+
integrity sha512-6aEYR910NyP73oHiJglti74iRyOwgFU4x3meH/H8OJx6Ry0j6cOVZ5X/wTvub7G7Ao6qaHBEaNsV3GLJkSsF+Q==
89+
dependencies:
90+
"@types/estree" "^1.0.0"
91+
estree-walker "^2.0.2"
92+
picomatch "^2.3.1"
93+
7294
"@types/estree@*", "@types/[email protected]":
7395
version "0.0.39"
7496
resolved "https://registry.npmjs.org/@types/estree/-/estree-0.0.39.tgz"
7597
integrity sha512-EYNwp3bU+98cpU4lAWYYL7Zz+2gryWH1qbdDTidVd6hkiR6weksdbMadyXKXNPEkQFhXM+hVO9ZygomHXp+AIw==
7698

99+
"@types/estree@^1.0.0":
100+
version "1.0.1"
101+
resolved "https://registry.yarnpkg.com/@types/estree/-/estree-1.0.1.tgz#aa22750962f3bf0e79d753d3cc067f010c95f194"
102+
integrity sha512-LG4opVs2ANWZ1TJoKc937iMmNstM/d0ae1vNbnBvBhqCSezgVUOzcLCqbI5elV8Vy6WKwKjaqR+zO9VKirBBCA==
103+
77104
"@types/json5@^0.0.29":
78105
version "0.0.29"
79106
resolved "https://registry.npmjs.org/@types/json5/-/json5-0.0.29.tgz"
@@ -1786,7 +1813,7 @@ estree-walker@^1.0.1:
17861813
resolved "https://registry.npmjs.org/estree-walker/-/estree-walker-1.0.1.tgz"
17871814
integrity sha512-1fMXF3YP4pZZVozF8j/ZLfvnR8NSIljt56UhbZ5PeeDmmGHpgpdwQt7ITlGvYaQukCvuBRMLEiKiYC+oeIg4cg==
17881815

1789-
estree-walker@^2.0.1:
1816+
estree-walker@^2.0.1, estree-walker@^2.0.2:
17901817
version "2.0.2"
17911818
resolved "https://registry.npmjs.org/estree-walker/-/estree-walker-2.0.2.tgz"
17921819
integrity sha512-Rfkk/Mp/DL7JVje3u18FxFujQlTNR2q6QfMSMB7AvCBx91NGj/ba3kCfza0f6dVDbw7YlRf/nDrn7pQrCCyQ/w==
@@ -3045,6 +3072,13 @@ magic-string@^0.25.7:
30453072
dependencies:
30463073
sourcemap-codec "^1.4.8"
30473074

3075+
magic-string@^0.30.3:
3076+
version "0.30.5"
3077+
resolved "https://registry.yarnpkg.com/magic-string/-/magic-string-0.30.5.tgz#1994d980bd1c8835dc6e78db7cbd4ae4f24746f9"
3078+
integrity sha512-7xlpfBaQaP/T6Vh8MO/EqXSW5En6INHEvEXQiuff7Gku0PWjU3uf6w/j9o7O+SpB5fOAkrI5HeoNgwjEO0pFsA==
3079+
dependencies:
3080+
"@jridgewell/sourcemap-codec" "^1.4.15"
3081+
30483082
make-dir@^2.0.0:
30493083
version "2.1.0"
30503084
resolved "https://registry.npmjs.org/make-dir/-/make-dir-2.1.0.tgz"
@@ -3573,7 +3607,7 @@ performance-now@^2.1.0:
35733607
resolved "https://registry.npmjs.org/performance-now/-/performance-now-2.1.0.tgz"
35743608
integrity sha512-7EAHlyLHI56VEIdK57uwHdHKIaAGbnXPiw0yWbarQZOKaKpvUIgW0jWRVLiatnM+XXlSwsanIBH/hzGMJulMow==
35753609

3576-
picomatch@^2.0.4, picomatch@^2.2.1, picomatch@^2.2.2:
3610+
picomatch@^2.0.4, picomatch@^2.2.1, picomatch@^2.2.2, picomatch@^2.3.1:
35773611
version "2.3.1"
35783612
resolved "https://registry.npmjs.org/picomatch/-/picomatch-2.3.1.tgz"
35793613
integrity sha512-JU3teHTNjmE2VCGFzuY8EXzCDVwEqB2a8fsIvwaStHhAWJEeVd1o1QD80CU6+ZdEXXSLbSsuLwJjkCBWqRQUVA==

0 commit comments

Comments
 (0)