hotwired / turbo-rails

Use Turbo in your Ruby on Rails app
https://turbo.hotwired.dev
MIT License
2.07k stars 320 forks source link

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

Closed davidrunger closed 2 years ago

davidrunger commented 2 years ago

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 https://github.com/hotwired/turbo-rails/pull/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 commented 2 years ago

cc @seanpdoyle (the author of https://github.com/hotwired/turbo-rails/pull/239)

dhh commented 2 years ago

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 commented 2 years ago

Hmm, I am on node 16.13.0 (both locally and on my GitHub Actions CI; see the failing dependabot upgrade in question here https://github.com/davidrunger/david_runger/pull/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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/hotwired/turbo-rails/pull/239 can/should be removed? Or maybe there is some other solution?

dhh commented 2 years ago

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.

KonnorRogers commented 2 years ago

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 commented 2 years ago

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.

KonnorRogers commented 2 years ago

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

https://github.com/webpack/webpack/issues/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 commented 2 years ago

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 https://github.com/hotwired/turbo-rails/pull/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. 🥲