Create a shareable ESLint configuration package #689

Merged
merged 13 commits into from Sep 21, 2016

Projects

None yet

2 participants

@fson
Collaborator
fson commented Sep 20, 2016

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?)

@ghost ghost added the CLA Signed label Sep 20, 2016
@@ -0,0 +1,21 @@
+{
@gaearon
gaearon Sep 20, 2016 Contributor

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"
@gaearon
gaearon Sep 20, 2016 Contributor

I would like to keep dependencies pinned, and update them manually. Our setup is a little more conservative.

@gaearon
gaearon Sep 20, 2016 Contributor

While we're at it, can you look into updating us to eslint-plugin-react@6? Have any rules changed?

@fson
fson Sep 20, 2016 Collaborator

I pinned the dependencies and added a README. I can look into updating the plugins tomorrow.

@gaearon

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
@fson
fson Sep 21, 2016 Collaborator

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 :(

@gaearon
gaearon Sep 21, 2016 Contributor

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.

@fson
fson Sep 21, 2016 Collaborator

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.

@fson
fson Sep 21, 2016 Collaborator

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.

@gaearon gaearon added this to the 0.5.0 milestone Sep 21, 2016
@ghost ghost added the CLA Signed label Sep 21, 2016
useEslintrc: false
},
+ // @remove-on-eject-end
@gaearon
gaearon Sep 21, 2016 Contributor

Don't we need this for prod config too?

@fson
fson Sep 21, 2016 Collaborator

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).

@gaearon
gaearon Sep 21, 2016 Contributor

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?

@fson
fson Sep 21, 2016 Collaborator

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",
@gaearon
gaearon Sep 21, 2016 Contributor

Wow, had no idea you could do that

@gaearon gaearon merged commit a2d0469 into facebookincubator:master Sep 21, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gaearon
Contributor
gaearon commented Sep 21, 2016

Thanks!

@fson fson deleted the fson:eslint-config-react-app branch Sep 21, 2016
@feiqitian feiqitian added a commit to feiqitian/create-react-app that referenced this pull request Oct 25, 2016
@fson @feiqitian fson + feiqitian Create a shareable ESLint configuration package (#689)
* 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
898fabc
@kst404 kst404 pushed a commit to kst404/e8e-react-scripts that referenced this pull request Dec 19, 2016
@fson @gaearon fson + gaearon Create a shareable ESLint configuration package (#689)
* 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
79fc251
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment