-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
Create a shareable ESLint configuration package #689
Conversation
@@ -0,0 +1,21 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll also want a readme here, explaining in a few words how to use this package as standalone.
"eslint-plugin-flowtype": "^2.18.1", | ||
"eslint-plugin-import": "^1.12.0", | ||
"eslint-plugin-jsx-a11y": "^2.2.2", | ||
"eslint-plugin-react": "^5.2.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to keep dependencies pinned, and update them manually. Our setup is a little more conservative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we're at it, can you look into updating us to eslint-plugin-react@6
? Have any rules changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pinned the dependencies and added a README. I can look into updating the plugins tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a README and pin deps
1. Install this package, ESLint and the necessary plugins: | ||
|
||
``` | ||
npm install eslint-config-react-app babel-eslint@6.1.2 eslint@3.5.0 eslint-plugin-flowtype@2.18.1 eslint-plugin-import@1.12.0 eslint-plugin-jsx-a11y@2.2.2 eslint-plugin-react@5.2.2 --save-dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having to install all the dependencies separately like this is ridiculous, but until ESLint supports plugin dependencies for shareable configs, there isn't anything we can do about it, I'm afraid :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do the same trick as here: https://github.com/airbnb/javascript/tree/master/packages/eslint-config-airbnb#eslint-config-airbnb-1. I'm just not sure what the Windows version would be like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could write it in JS and have a node -e '<SCRIPT>'
here, so it would be the same for Windows? I assume everyone who is going to use this will have node
installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having the command that you can copy paste to your console here is better than that trick, as long as we keep the version numbers up-to-date. It's a bit more work for us, but easier for the users.
useEslintrc: false | ||
}, | ||
// @remove-on-eject-end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need this for prod config too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We copy the .eslintrc
to the ejected project, so we don't need to specify a custom configFile
path or to disable the .eslintrc
. I think people prefer the rc files for configuration in an ejected project (see #410).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't mean ejected, I meant webpack.config.prod.js
. Don't we need to update path in it as well as .dev.js
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, great point. I forgot we had ESLint for the build too. Updated now.
"eslint": "3.5.0", | ||
"eslint-config-react-app": "file:packages/eslint-config-react-app", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, had no idea you could do that
Thanks! |
* Move ESLint configuration to a separate package * Remove the ESLint configuration, moved to eslint-config-react-app * Update ESLint instructions * Pin the package versions in eslint-config-react-app * Add a README for eslint-config-react-app * Update README.md * Update README.md * Update README.md * Update README.md * Update package.json * Update package.json * Update production eslint-loader config * Add the ESLint config to devDependencies of the repo
Move the ESLint configuration to a separate package
eslint-config-react-app
. This allows using the same configuration in projects not using CRA. It also makes the amount of configuration in an ejected project smaller: the starter ESLint config of an ejected the project will be just{"extends": "react-app"}
.Test plan:
[x] Check that linting errors are shown correctly when running
npm start
.[x] Check that it also works end to end. (Tested with a local npm server.)
(NB: the e2e test will break at the moment because the package hasn't been published yet, and it tries to install it from npm. However, I tested by publishing it to a local npm registry with sinopia, and it worked. I think we might want to come up with a way to end-to-end test all the packages together without having to manually pack all of them in the test script. But for now pulling the linter config from npm is probably good enough?)