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

Upgrading from v2.0.8 to 3.0.0 breaks build #25

Closed
barbalex opened this issue Feb 2, 2021 · 8 comments
Closed

Upgrading from v2.0.8 to 3.0.0 breaks build #25

barbalex opened this issue Feb 2, 2021 · 8 comments

Comments

@barbalex
Copy link

barbalex commented Feb 2, 2021

I did the following in a working project:

  • yarn add --dev eslint-webpack-plugin
  • bumped gatsby-plugin-eslint to 3.0.0 in package.json
  • yarn && yarn upgrade && yarn dev

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
Copy link

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
Copy link
Author

barbalex commented Feb 3, 2021

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

@chris-erickson
Copy link

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
Copy link
Owner

mongkuen commented Feb 4, 2021

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

@mongkuen mongkuen closed this as completed Feb 4, 2021
@mongkuen mongkuen pinned this issue Feb 4, 2021
@pieh
Copy link

pieh commented Feb 5, 2021

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 #25 (comment) 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 gatsbyjs/gatsby#29317 and my comment/approval on that PR with me testing those various scenarios.

@barbalex
Copy link
Author

barbalex commented Feb 5, 2021

Thanks a lot @pieh
Unfortunately I now run into gatsbyjs/gatsby#29326 (which is out of the scope of this issue, I realize)

@barbalex
Copy link
Author

barbalex commented Feb 5, 2021

@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
Copy link
Owner

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.

@mongkuen mongkuen unpinned this issue Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants