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

Module parse failed: Unexpected token when transpiling with webpack due to inclusion of TypeScript optional chaining operator (submitter?.formMethod) #336

Closed
davidrunger opened this issue May 23, 2022 · 10 comments · Fixed by #337

Comments

@davidrunger
Copy link

davidrunger commented May 23, 2022

I am attempting to bump @hotwired/turbo-rails from 7.1.1 to 7.1.2, but I am getting this error when running bin/webpack in my Rails app:

ERROR in ./node_modules/@hotwired/turbo-rails/app/javascript/turbo/form_submissions.js 9:31
Module parse failed: Unexpected token (9:31)
File was processed with these loaders:
 * ./node_modules/babel-loader/lib/index.js
You may need an additional loader to handle the result of these loaders.
|   }
| }) {
>   const formMethod = submitter?.formMethod;
|
|   if (formMethod && fetchRequest.body.has("_method")) {
 @ ./node_modules/@hotwired/turbo-rails/app/javascript/turbo/index.js 2:0-66 7:39-67
 @ ./app/javascript/packs/quizzes.js

It seems that the error is being caused by the inclusion of the TypeScript optional chaining operator (?.) in that file (app/javascript/turbo/form_submissions.js) in submitter?.formMethod.

I think that this issue was introduced in #239 (Support [formmethod] overrides to _method, which was merged yesterday and released recently) here: https://github.com/hotwired/turbo-rails/pull/239/files#diff-f438134433fd9897751d1b5cd219ea33ac29be8f3af3e4ac17e65475e4cadbeeR2.

@davidrunger
Copy link
Author

cc @seanpdoyle (the author of #239)

@dhh
Copy link
Member

dhh commented May 23, 2022

The chaining operator should be supported from Node 14+. I think we should probably still address this, but curious what version you're on? Maybe we need to find a way to have version requirements.

@davidrunger
Copy link
Author

davidrunger commented May 23, 2022

Hmm, I am on node 16.13.0 (both locally and on my GitHub Actions CI; see the failing dependabot upgrade in question here davidrunger/david_runger#1278 and the CI output including node version verification and the Module parse failed error here: https://github.com/davidrunger/david_runger/runs/6555766119).

Maybe there is some flag/setting/option that I'd need to turn on in order for the optional chaining operator to work? (Edit: inversely, maybe I have enabled some flag that disables parsing the optional chaining operator, and I need to turn that off?)

@davidrunger
Copy link
Author

Maybe I should also mention that I am using webpacker (version 5.4.3), which I know has been retired. I'm not sure if that might be a factor.

@davidrunger
Copy link
Author

davidrunger commented May 23, 2022

Inspired by this Stack Overflow answer, I am able to get webpack to compile successfully by adding the following to my webpack configuration (which, for my app, is located in config/webpack/shared.js):

// ...
module.exports = {
  // ...
  module: {
    rules: [
      {
        test: /\.js$/,
        loader: 'babel-loader',
        options: {
          plugins: [
            '@babel/plugin-proposal-optional-chaining',
          ],
        },
      },
      // ...
    ],
  },
  // ...
};

(Edit: note that I do already have a .babelrc.js file which includes @babel/preset-env, which I think includes support for @babel/plugin-proposal-optional-chaining for my app's own JavaScript. But I think that the .babelrc.js file is not used when processing files in node_modules, which is why I think that the above configuration change is necessary, despite my preexisting .babelrc.js configuration.)

However, doing this (i.e. adding a webpack rule for .js files and using babel to parse .js files in node_modules, which I think babel doesn't do by default) seems to introduce a decently significant performance penalty (raising the bin/webpack time on my machine from ~40 seconds to ~56 seconds).

Additionally, it's a bit inconvenient (and I suspect not documented by @hotwired/turbo-rails?) that webpack users seemingly must add something like that to their webpack configuration, if it's not already there.

I'm curious to know whether I am unique in my setup and encountering this error vs. whether it is affecting / will affect many/most other users upgrading to @hotwired/turbo-rails 7.1.2. If it's a unique problem that I am having, then maybe I'll just add that to my webpack configuration and close this issue. However, if it's a problem that others are having, too, maybe the optional chaining operator added in #239 can/should be removed? Or maybe there is some other solution?

@dhh
Copy link
Member

dhh commented May 23, 2022

Yeah, easiest path here seems to be just rewriting that optional chaining use to not use that. Do take a stab if @seanpdoyle doesn't beat you to it.

seanpdoyle added a commit to seanpdoyle/turbo-rails that referenced this issue May 23, 2022
Closes [hotwired#336][]

Address downstream Rails consumers' webpack syntax issues by replacing
the `?` operator with a check that `submitter` is present.

[hotwired#336]: hotwired#336
@KonnorRogers
Copy link

As far as I know, this would be a Babel issue, not a Webpack issue.

Upgrading the Babel version / Babel parser should fix this. Optionally, you can tell Webpack not to transform Turbo. This isn't even TypeScript or Node specific.

optional chaining is supported by the browser and has very good support.

https://caniuse.com/?search=optional%20chaining

just my 2 cents.

@davidrunger
Copy link
Author

I am using the latest version of @babel/core (7.18.0). Here it is listed in the yarn.lock file of the app where I am experiencing this issue: https://github.com/davidrunger/david_runger/blob/dependabot/npm_and_yarn/hotwired/turbo-rails-7.1.2/yarn.lock#L47-L48. So I don't think that upgrading the babel version will fix this; I think that I cannot upgrade the babel version, since I am already using the latest version.

It seems that my babel version (the latest) can parse the @hotwired/turbo-rails files, and it does when I configure webpack as mentioned in my comment above, but babel doesn't seem to do this by default (or webpack/webpacker declines to use babel to do this by default), which is seemingly for a somewhat good reason, since having babel parse all .js files seems to come with a transpilation performance penalty.

Telling webpack not to transform Turbo might work, but that seems to me like something that would be a little bit of friction for @hotwired/turbo-rails users that it'd be better if they can avoid, and also I guess that we'd lose benefits like tree shaking that come via webpack.

@dhh dhh closed this as completed in #337 May 24, 2022
dhh pushed a commit that referenced this issue May 24, 2022
Closes [#336][]

Address downstream Rails consumers' webpack syntax issues by replacing
the `?` operator with a check that `submitter` is present.

[#336]: #336
@KonnorRogers
Copy link

KonnorRogers commented May 24, 2022

@davidrunger i believe I found the root of the problem. The parser used under the hood (acorn) didn't support optional chaining until v7.3.0.

Here's an issue thread that outlines it:

webpack/webpack#10227

TLDR: Webpack 4 (which I believer webpacker 5.4.3 uses) doesn't support it without adding optional chaining to Babel, or setting an acorn resolution.

@davidrunger
Copy link
Author

davidrunger commented May 24, 2022

Awesome digging @ParamagicDev , and thank you for figuring out what is causing this!

It looks like you are correct that @rails/webpacker 5.4.3 (the latest non-beta/pre/rc release, which I am using) depends on and, at least in my app, uses webpack 4 (^4.46.0), which in turn uses acorn@^6.4.1 (which resolves to 6.4.2), which indeed doesn't support optional chaining, as you noted and as discussed in that issue you linked.

Forcing yarn to resolve acorn to the latest version by adding the following to my package.json and running yarn install does indeed fix this bin/webpack error that I had been getting with @hotwired/turbo-rails 7.1.2.

{
  // ...
  "resolutions": {
    "acorn": "8.7.1"
  }
}

(So, luckily, apparently whatever breaking changes have been introduced between acorn 6.4.2 and acorn 8.7.1 don't break the webpack transpilation process in my app. That's not a totally perfect/sustainable solution, though, since I don't like to see warnings, and when doing the above and running yarn install I do see a warning about warning Resolution field "acorn@8.7.1" is incompatible with requested version "acorn@^6.4.1", and when running bin/webpack I see a warning about Since Acorn 8.0.0, options.ecmaVersion is required. Defaulting to 2020, but this will stop working in the future.. Seeing these warnings is not a major problem, though.)

I do feel a little guilty about encountering this error and raising it here, when ultimately the source of the error is essentially the fact that I am still using webpacker, which has been retired and is getting more outdated by the day. Still, I guess that there are probably others like me out there who are using @hotwired/turbo-rails while still using webpacker, so hopefully removing the optional chaining operator as has been done in #337 (and released via @hotwired/turbo-rails 7.1.3) to fix/avoid this issue for them and me hasn't been a total waste.

This does provide some extra motivation, though, for me to bite the bullet and do some work to stop using webpacker, so that I won't be the laggard encountering and reporting issues like this in the future. 🥲

atosbucket added a commit to atosbucket/turbo-rails that referenced this issue Apr 11, 2024
Closes [#336][]

Address downstream Rails consumers' webpack syntax issues by replacing
the `?` operator with a check that `submitter` is present.

[#336]: hotwired/turbo-rails#336
newheaven918 added a commit to newheaven918/turbo that referenced this issue Apr 26, 2024
Closes [#336][]

Address downstream Rails consumers' webpack syntax issues by replacing
the `?` operator with a check that `submitter` is present.

[#336]: hotwired/turbo-rails#336
luisjose1996 added a commit to luisjose1996/Turbo-Rails that referenced this issue May 10, 2024
Closes [#336][]

Address downstream Rails consumers' webpack syntax issues by replacing
the `?` operator with a check that `submitter` is present.

[#336]: hotwired/turbo-rails#336
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants