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 6 support #7513

Merged
merged 2 commits into from
Aug 13, 2019
Merged

Fix ESLint 6 support #7513

merged 2 commits into from
Aug 13, 2019

Conversation

ianschmitz
Copy link
Contributor

Fixes #7510.

extends is no longer available in ESLint 6.

Copy link
Contributor

@mrmckeb mrmckeb left a comment

Choose a reason for hiding this comment

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

One question, if that's OK - good to merge! Thanks @ianschmitz

} catch (e) {
// A config couldn't be found.
}
const eslintConfig = eslintCli.getConfigForFile(
Copy link
Contributor

@mrmckeb mrmckeb Aug 12, 2019

Choose a reason for hiding this comment

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

This threw an error previously if a file wasn't found. I understand it doesn't now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I may have been getting lucky (eslint config somewhere up the file system tree perhaps?). I was seeing errors in CI so i've reverted to include the catch again.

I also observed a strange error on our PNP test that failed due to eslint-config-react-app not being defined in package.json. This catch block is catching that error on master it appears.

@ianschmitz ianschmitz requested a review from amyrlam as a code owner August 12, 2019 20:20
Copy link
Contributor

@n1ru4l n1ru4l left a comment

Choose a reason for hiding this comment

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

Please don't swallow the eslint config error. I think it would be better to throw the error incase the EXTEND_ESLINT environment variable is set.

E.g. I had some issues where a plugin referenced in the eslint config was not installed. I had to manually edit this file in order to print the error...

I would prefer a solution similar to this:

#7510 (comment)

@mAAdhaTTah
Copy link

mAAdhaTTah commented Aug 13, 2019

You could also solve @n1ru4l's issue by only looking for a config file if EXTEND_ESLINT is set in the first place. Otherwise, it does a search for the file always but ignores it if the env var isn't set, which is a minor perf issue if you're not extending the config.

The other issue with this PR is the original wanted to ensure the react-app config was included, which this no longer does. Personally, I think if you're extending the ESLint config, you know what you're doing, and this should be allowed, but that doesn't seem to be in line with the project's philosophy.

@avindra
Copy link

avindra commented Aug 15, 2019

This is great work all around. Thanks everyone. I sort of wish EXTEND_ESLINT would also disable the linting at build time (so that linting be handled separately (or in a different order) in people's build systems, for example). Is there any hope of such separation for create-react-app?

@lock lock bot locked and limited conversation to collaborators Aug 21, 2019
@ianschmitz ianschmitz deleted the eslint-fix branch September 13, 2019 23:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extending ESLint config not working
7 participants