mongkuen / gatsby-plugin-eslint

Gatsby plugin to add support for ESLint
MIT License
13 stars 12 forks source link

Upgrading from v2.0.8 to 3.0.0 breaks build #25

Closed barbalex closed 3 years ago

barbalex commented 3 years ago

I did the following in a working project:

The result is hundreds if not thousands of files failing the build. Here an example:

 ERROR #98123  WEBPACK

Generating development JavaScript bundle failed

C:\Users\alexa\apf2\src\components\Projekte\Daten\Tpop\History.js
  175:20  error  Parsing error: Unexpected token .

✖ 1 problem (1 error, 0 warnings)

When I check the code mentioned it is always the usage of ?. for optional chaining, for example in above file:

const row = data?.tpopById

It is possible that other modern js methods cause this too, I haven't checked too many files.

RevereMarketing commented 3 years ago

Hey @barbalex, we are having the same issue. Did you find a temporal solution for this? The only thing working on our end is removing the optional chaining

barbalex commented 3 years ago

@RevereMarketing I remain on v2.0.8. Removing optional chaining is not an option due to it's extensive use.

chris-erickson commented 3 years ago

Is this at all related to these issues? It just seems like babel is no longer properly integrated in some situations and they must not be that rare to get a warning like this. Just wish there was more context around why this was known to break something and how one might fix it. Especially since it seems like a recommended way towards linting a Gatsby site.

mongkuen commented 3 years ago

This is quite a bit outside the scope of the plugin (the source is literally 15 lines of code). I'm not a Gatsby contributer, and honestly haven't even used Gatsby since v1. Gatsby's automagic linting is news to me - and this plugin probably doesn't even need to exist anymore seeing as it's already built in 😅

But digging a little I've diagnosed the issue - and I'll let @pieh be the judge of whether or not this is already fixed, or if something more needs to be done on Gatsby's side.

Specifically in your project @barbalex this is an issue with Gatsby 2.31.1

If anything, the v2 plugin was SUPPRESSING the issue, and the upgrade to v3 seemed to "break" your build because it no longer suppressed the problem. You can comment out the v2 plugin from your gatsby-config file and watch your build break.

Specifically, the eslintConfig in your package.json will flip hasLocalEslint to true, and invoke ensureRequireEslintRules

https://github.com/gatsbyjs/gatsby/blob/32c20cf835b4a5d7732ccba25f4bafe4ce7a0e6b/packages/gatsby/src/utils/webpack.config.js#L737-L743

https://github.com/gatsbyjs/gatsby/blob/32c20cf835b4a5d7732ccba25f4bafe4ce7a0e6b/packages/gatsby/src/utils/webpack-utils.ts#L767-L800

Inside ensureRequireEslintRules, it does a hardcoded check for eslint-loader. When using the v2 plugin, it will merge into the rule and everything will be fine.

However without an existing rule, it will create it's own rule with the options in eslintRequiredConfig. This will break your build because it's using parserOptions of ecmaVersion: 2018, which doesn't support optional chaining (an ecma 2020 feature).

https://github.com/gatsbyjs/gatsby/blob/32c20cf835b4a5d7732ccba25f4bafe4ce7a0e6b/packages/gatsby/src/utils/eslint-config.ts#L8-L26

TLDR: The reason upgrading to v3 of the plugin breaks your build, is that eslint-loader has been removed, and Gatsby creates it's own rule using an old ecmaVersion: 2018 parser that doesn't support option chaining.


So, with all that said. What to do about it? At this point for you, there probably two options that make the most sense:

  1. Nothing: No need to fix what isn't broken, and there's no compelling reason to upgrade. I released v3 because I got several requests to remove the deprecated eslint-loader
  2. Try upgrading Gatsby: This may or may not solve your issue, and may/may not break your build in other different ways. Your call there.

Closing, but pinning the issue for anyone else who runs into this problem

pieh commented 3 years ago

The bump that caused

C:\Users\alexa\apf2\src\components\Projekte\Daten\Tpop\History.js
  175:20  error  Parsing error: Unexpected token .

was in gatsby core package, not this plugin.

As @chris-erickson pointed out in https://github.com/mongkuen/gatsby-plugin-eslint/issues/25#issuecomment-772848134 we introduced regression in 2.31.0 / 231.1 and just merged another fix that I tested with various versions of gatsby-plugin-eslint (both version 2 that used eslint-loader and version 3 that use eslint-webpack-plugin) with optional chaining etc and released gatsby@2.31.2 yesterday - please check https://github.com/gatsbyjs/gatsby/pull/29317 and my comment/approval on that PR with me testing those various scenarios.

barbalex commented 3 years ago

Thanks a lot @pieh Unfortunately I now run into https://github.com/gatsbyjs/gatsby/issues/29326 (which is out of the scope of this issue, I realize)

barbalex commented 3 years ago

@pieh During the last few months approximately 50% of the time I updated gatsby dependencies I have encountered hard to solve errors somewhat like this. Gatsby has dived deep into js dependency hell. To me I am on the verge of starting the next project with a different base (next.js?). I realize it hits you much harder. I appreciate your great feedback and efforts!

mongkuen commented 3 years ago

Just an FYI to @pieh, I've rewritten this plugin to work with V3 now that eslint-loader has been fully removed.

Broad strokes.

  1. Take current webpack config
  2. Uses webpack-merge's unique to ensure only the user's custom ESLint config is loaded. This means all pre-existing instances of ESLintPlugin (Including Gatsby's eslintRequiredConfig) are removed.
  3. Uses actions.replaceWebpackConfig to inject modified webpack config

Users have instructions on how to re-enable the required rules. The basic approach I supplied is to just add another rulePath to eslint-webpack-plugin via

// gatsby-config.js
{
  resolve: "gatsby-plugin-eslint",
  options: {
    rulePaths: [path.join(process.cwd(), "node_modules", "gatsby", "dist", "utils", "eslint-rules" )]
  }
}

and

// .eslintrc
{
  "rules": {
    "no-anonymous-exports-page-templates": "warn",
    "limited-exports-page-templates": "warn"
  }
}

Hopefully everything seems reasonable. Let me know if this might be undesirable or if there is a better approach.