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

only load eslint config when EXTEND_ESLINT environment variable is specified/ do not swallow eslint config errors #7530

Merged
merged 2 commits into from Sep 27, 2019

Conversation

@n1ru4l
Copy link
Contributor

n1ru4l commented Aug 14, 2019

Based on the following discussion that got no response:

#7513 (review)
#7513 (comment)
#7510 (comment)

Summary:

Swallowing the eslint config error is not a good idea

The line eslintConfig = eslintCli.getConfigForFile(paths.appIndexJs); can throw in case the provided eslint config is invalid, e.g. when there is a typo or a plugin dependency is missing. Swallowing that error in case the user explicitly sets the EXTEND_ESLINT environment variable (and therefore expects that his eslint config is used) makes debugging such issues unnecessary cumbersome.

The eslint config should only be loaded in case the user wants to load it

The eslint config is never used in case the EXTEND_ESLINT is not set. Therefore it makes sense to only resolve it's path in case the environment variable is set.

…ecified
@mrmckeb

This comment has been minimized.

Copy link
Collaborator

mrmckeb commented Aug 14, 2019

This is a good change, thanks. I'll try to take a look next week (I'm on a business trip now) - unless someone else takes a look before then.

The performance issue would be very small though, as this only runs one time at start.

@n1ru4l

This comment has been minimized.

Copy link
Contributor Author

n1ru4l commented Aug 26, 2019

Hey, @mrmckeb, it would be awesome if you could review this pull request once you got some time for it! I still think that this is a major pain-point for using a custom eslint config.

@ianschmitz

This comment has been minimized.

Copy link
Collaborator

ianschmitz commented Sep 13, 2019

@n1ru4l do you mind resolving the conflict?

@ianschmitz ianschmitz added this to the 3.x milestone Sep 13, 2019
@n1ru4l

This comment has been minimized.

Copy link
Contributor Author

n1ru4l commented Sep 15, 2019

@ianschmitz done 👍

@klaaspieter

This comment has been minimized.

Copy link

klaaspieter commented Sep 26, 2019

I believe this fixes #7552

Copy link
Collaborator

mrmckeb left a comment

This looks great, thanks!

@mrmckeb mrmckeb merged commit 6533a6d into facebook:master Sep 27, 2019
25 checks passed
25 checks passed
facebook.create-react-app Build #20190915.1 succeeded
Details
facebook.create-react-app (Behavior LinuxNode10) Behavior LinuxNode10 succeeded
Details
facebook.create-react-app (Behavior LinuxNode8) Behavior LinuxNode8 succeeded
Details
facebook.create-react-app (Behavior MacNode10) Behavior MacNode10 succeeded
Details
facebook.create-react-app (Behavior MacNode8) Behavior MacNode8 succeeded
Details
facebook.create-react-app (Behavior WindowsNode10) Behavior WindowsNode10 succeeded
Details
facebook.create-react-app (Behavior WindowsNode8) Behavior WindowsNode8 succeeded
Details
facebook.create-react-app (Installs LinuxNode10) Installs LinuxNode10 succeeded
Details
facebook.create-react-app (Installs LinuxNode8) Installs LinuxNode8 succeeded
Details
facebook.create-react-app (Installs WindowsNode10) Installs WindowsNode10 succeeded
Details
facebook.create-react-app (Installs WindowsNode8) Installs WindowsNode8 succeeded
Details
facebook.create-react-app (Kitchensink LinuxNode10) Kitchensink LinuxNode10 succeeded
Details
facebook.create-react-app (Kitchensink LinuxNode8) Kitchensink LinuxNode8 succeeded
Details
facebook.create-react-app (Kitchensink WindowsNode10) Kitchensink WindowsNode10 succeeded
Details
facebook.create-react-app (Kitchensink WindowsNode8) Kitchensink WindowsNode8 succeeded
Details
facebook.create-react-app (KitchensinkEject LinuxNode10) KitchensinkEject LinuxNode10 succeeded
Details
facebook.create-react-app (KitchensinkEject LinuxNode8) KitchensinkEject LinuxNode8 succeeded
Details
facebook.create-react-app (KitchensinkEject WindowsNode10) KitchensinkEject WindowsNode10 succeeded
Details
facebook.create-react-app (KitchensinkEject WindowsNode8) KitchensinkEject WindowsNode8 succeeded
Details
facebook.create-react-app (OldNode) OldNode succeeded
Details
facebook.create-react-app (Simple LinuxNode10) Simple LinuxNode10 succeeded
Details
facebook.create-react-app (Simple LinuxNode8) Simple LinuxNode8 succeeded
Details
facebook.create-react-app (Simple WindowsNode10) Simple WindowsNode10 succeeded
Details
facebook.create-react-app (Simple WindowsNode8) Simple WindowsNode8 succeeded
Details
netlify/create-react-app/deploy-preview Docs deploy preview succeeded
Details
@lock lock bot locked and limited conversation to collaborators Oct 2, 2019
@n1ru4l n1ru4l deleted the n1ru4l:patch-1 branch Oct 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.