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

Extending ESLint is not supporting overrides #7776

Closed
Andarist opened this issue Oct 3, 2019 · 12 comments · Fixed by #8276
Closed

Extending ESLint is not supporting overrides #7776

Andarist opened this issue Oct 3, 2019 · 12 comments · Fixed by #8276
Assignees
Milestone

Comments

@Andarist
Copy link
Contributor

Andarist commented Oct 3, 2019

Extending ESLint got introduced originally in #7036 by @mrmckeb. It's stated that this is limited currently to users configuring their ESLint in package.json:

  • this ain't quite true - because used eslintCli.getConfigForFile is not limited to package.json configs
  • there is no such mention in the docs - not a biggie but brought a little bit of confusion for me.

What is actually stated in the docs and what doesn't actually hold true is that overrides can be used to support TS rules etc (there is example of that).

This is not quite true, because whatever config gets loaded & resolved through that eslintCli.getConfigForFile call (with paths.appIndexJs as argument) gets set as "global" config for the whole eslint-loader.

So depending on what paths.appIndexJs is we either can get:

  • all TS-related rules being applied to ALL files (including JS) - when paths.appIndexJs is TS
  • no TS rules are applied at all - when paths.appIndexJs is JS

For this to work correctly it would be the easiest to allow ESLint to load configs on its own - without passing explicit single config to the eslint-loader.

@shadel
Copy link

shadel commented Oct 4, 2019

i have same issue

@stale
Copy link

stale bot commented Nov 3, 2019

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Nov 3, 2019
@ae2438
Copy link

ae2438 commented Nov 3, 2019

This also leads to other problems, such as the same parser being used for JS and TS files. This leads to parsing errors in mixed code bases which is an annoying problem because syntactically correct code can't be run due to a non essential tool (ESLint) which can't even be turned off.

@stale stale bot removed the stale label Nov 3, 2019
@ianschmitz ianschmitz added this to the 3.3 milestone Nov 3, 2019
@ianschmitz ianschmitz modified the milestones: 3.3, 3.4 Dec 5, 2019
@ianschmitz
Copy link
Contributor

Hi @Andarist. Sorry this fell through the cracks. Let me try to address your comments:

Extending ESLint got introduced originally in #7036 by @mrmckeb. It's stated that this is limited currently to users configuring their ESLint in package.json:

Agreed. This should be clarified in the docs.

What is actually stated in the docs and what doesn't actually hold true is that overrides can be used to support TS rules etc (there is example of that).

Yeah this should be explicitly stated in the language above that example.

So depending on what paths.appIndexJs is we either can get:

  • all TS-related rules being applied to ALL files (including JS) - when paths.appIndexJs is TS
  • no TS rules are applied at all - when paths.appIndexJs is JS

Can you confirm this? I seem to remember when testing this that getConfigForFile() resolves the entire configuration (both TS, and JS) irrespective of the file extension. That would be the intent anyways so that we support mixed JS/TS code bases.

Thanks for bringing this up. We'd gladly accept PRs to improve the documentation for points you raised.

@mrmckeb
Copy link
Contributor

mrmckeb commented Dec 19, 2019

Hi all, sorry for inconvenience. This was something I briefly tested during development, but admittedly it needed more work.

I'm not sure how to best go about this, I have an idea... but have reached out to the ESLint team for advice.
eslint/eslint#12685

@mrmckeb
Copy link
Contributor

mrmckeb commented Dec 19, 2019

@Andarist, sorry I think we were confused about what you were reporting.

I've managed to reproduce now and understand. Sorry for the inconvenience.

So, in short:

  • The config itself is fine.
  • Instructions need to be updated.
  • Return undefined to baseConfig when process.env.EXTEND_ESLINT === 'true'.
  • Change useEslintrc: false to useEslintrc: process.env.EXTEND_ESLINT === 'true'.

How do these changes sound to everyone? I've tested them by modifying the Webpack config (in node_modules directly) - if others can do the same and report back, I can make the change this weekend.

@Andarist
Copy link
Contributor Author

@mrmckeb Thanks for taking a look at this! I believe that would help to fix my case because it would simply offload config loading to ESLint and everything would be loaded on per-file basis 👍

@eip1599
Copy link
Contributor

eip1599 commented Dec 29, 2019

@mrmckeb I was also struggling for days with this issue. Yes, that solves the problem.
But if this is the only way to work around, I think process.env.EXTEND_ESLINT should be changed to process.env.USE_ESLINTRC or something.

@eip1599
Copy link
Contributor

eip1599 commented Dec 30, 2019

@mrmckeb But when I applied your changes and set
"eslintConfig": { "extends": "" },, eslint still comes from somewhere. That strange as it works different when I set that without your change.

@mrmckeb
Copy link
Contributor

mrmckeb commented Jan 3, 2020

Hi @dev-xu, we didn't design this functionality to allow you to pass in no config. So, at this stage that's not supported. The empty string should fail... and I'm surprised that it falls back to the config.

Perhaps #8276 will solve that for you.

@mrmckeb
Copy link
Contributor

mrmckeb commented Jan 3, 2020

Sorry @Andarist, Christmas has eaten up a lot of time. I can take a look at this over the coming days - or you can raise a PR if you'd like? Let me know!

Edit: I've created a PR here - #8276

If anyone can take a look and let me know if this resolves your problems, that would be great.

@Andarist
Copy link
Contributor Author

Andarist commented Jan 3, 2020

Thank you @mrmckeb for a fix! I'm afraid I won't be able to test this out within the next 2 weeks as I'm on parental leave right now.

@ianschmitz ianschmitz modified the milestones: 3.3.1, 3.4 Jan 31, 2020
@iansu iansu modified the milestones: 3.4, 3.5 Feb 14, 2020
@ianschmitz ianschmitz modified the milestones: 3.5, 3.4.1 Feb 19, 2020
@lock lock bot locked and limited conversation to collaborators Mar 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants