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

fix(eslint-config): set parser, don't fail the build on minimal eslint-loader errors #29130

Closed
wants to merge 1 commit into from

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Jan 21, 2021

Description

#29109 was not sufficent. This also specify parser (by default it uses https://github.com/eslint/espree - but it seems like version we/eslint (also version of eslint we use) doesn't support optional chaining ?. - used version is espree@^6, and according to https://github.com/eslint/espree/blob/master/CHANGELOG.md support was added in espree@^7).

Other part is emitWarning: true, which should prevent errors coming from eslint-loader from blocking users - per https://github.com/webpack-contrib/eslint-loader#noemitonerrorsplugin

NoEmitOnErrorsPlugin is now automatically enabled in webpack 4, when mode is either unset, or set to production. So even ESLint warnings will fail the build. No matter what error settings are used for eslint-loader, except if emitWarning enabled.

Current:
Screenshot 2021-01-21 at 20 30 47

With emitWarning: true - so it still is confusing, and we need to fix those, but at least it doesn't prevent webpack from compiling (or rather emitting I guess):
Screenshot 2021-01-21 at 20 30 36

It prooved to be whack'a'mole with this minimal eslint config for fast-refresh rules, so there are few things to consider here:

  • maybe we should disable it (for now)?
  • lack of tests for this code path doesn't inspire confidence

Related Issues

#29105 (comment)

Fixes #29133

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jan 21, 2021
@theskillwithin
Copy link
Contributor

I am experiencing this as well as long with Parsing error: Unexpected token = on old class components

@LekoArts LekoArts added topic: resource loading* topic: cli Related to the Gatsby CLI and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer topic: resource loading* labels Jan 25, 2021
@mikaelkristiansson
Copy link
Contributor

what is the progress of this?

baseConfig: {
parser: require.resolve(`babel-eslint`),
Copy link
Contributor

@wardpeet wardpeet Jan 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we add it to the package.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already have it -

"babel-eslint": "^10.1.0",

baseConfig: {
parser: require.resolve(`babel-eslint`),
parserOptions: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for ecmaVersion I set to 2020. any reason why its 2018 here?

  parserOptions: {
    ecmaVersion: 2020,
    sourceType: 'module',
    ecmaFeatures: {
      jsx: true,
    },
  },

@vladar
Copy link
Contributor

vladar commented Feb 2, 2021

I don't think it fixes issue #29133

The problem is that we add our required config here:

if (
stage === `develop` &&
process.env.GATSBY_HOT_LOADER === `fast-refresh` &&
hasLocalEslint(program.directory)
) {
finalConfig = ensureRequireEslintRules(finalConfig)
}

But after #29109 this required config has useEslintrc: false. So we effectively disable eslintrc with our new settings, i.e. even if the file exists, the loader ignores it with our config.

So to recap - this PR indeed fixes the problem with parser but not the eslintrc part.

As for me, it sounds like the only good way to fix this is to have two eslint loaders - one for normal eslint setup - and another one - for specific fast-refresh rules. That separate loader can have any config, e.g. we can safely disable eslintrc and inline comments for it.

@pieh
Copy link
Contributor Author

pieh commented Feb 3, 2021

Closing this in favour of #29317 which is more robust

@pieh pieh closed this Feb 3, 2021
@LekoArts LekoArts deleted the minimal-eslint-vol-2 branch April 23, 2021 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: cli Related to the Gatsby CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gatsby 2.30.0 to 2.31.0 breaks .eslintrc.js
6 participants