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

Move ESLint configuration to an ESLint plugin #993

Closed
wants to merge 1 commit into from

Conversation

fson
Copy link
Contributor

@fson fson commented Oct 31, 2016

Work in progress, not ready for merging before the issues have been fixed and we've decided if we want this change.

Not being able to depend on ESLint plugins in our shareable config has been a long-standing issue (#173, #649, #866).

Adding support for this in ESLint is being discussed in eslint/eslint#3458. In eslint/eslint#3458 (comment), @nzakas suggested that the problem of bundling plugins as dependencies could be solved using an ESLint plugin instead of a shareable config. This PR is a first attempt to implement Create React App ESLint configuration as a plugin.

Breaking changes if we implement this change (for projects using eslint-config-react-app directly, e.g. ejected projects):

  • users of eslint-config-react-app will have to switch the dependency to eslint-plugin-react-app and replace "extends": "react-app" with"extends": "plugin:react-app/recommended"`
  • users of eslint-config-react-app should remove import, flowtype, jsx-a11y and react plugin dependencies because we will bundle them in our plugin
  • users of eslint-config-react-app that have extended the configuration will have to prefix plugin specific rules, e.g. "react/no-is-mounted": "error" becomes "react-app/react/no-is-mounted": "error"

Remaining issues:

  • Currently getting an error when trying to start the app: Error: Failed to load plugin react-app: Cannot find module 'eslint-plugin-react-app' Referenced from: /Users/ville/Projects/create-react-app/my-es-app/node_modules/react-scripts/.eslintrc, so I'm not sure if ESLint will be able to resolve the plugin correctly (especially if it's been bundled inside react-scripts).
  • Editor integrations need to be tested and documentation changed.

@fson
Copy link
Contributor Author

fson commented Nov 8, 2016

It just occurred to me that this might not solve the issues with editor integrations. The users would not be installing the plugin, they would be installing react-scripts that has the plugin as a dependency:

 react-scripts
 └─ eslint-plugin-react-app
    ├─ eslint-plugin-react
    ├─ eslint-plugin-flow
    └─ ... etc

Now npm@3+ and yarn would flatten these dependencies, so it might work (at least if we get rid of bundledDependencies), but would it be hacky to rely on that? Also it seems that the situation would be the same with the current shareable config, if we would remove bundled deps, so I'm not sure if switching to a plugin would have any significant effect.

@gaearon
Copy link
Contributor

gaearon commented Nov 20, 2016

Also it seems that the situation would be the same with the current shareable config, if we would remove bundled deps, so I'm not sure if switching to a plugin would have any significant effect.

Fair enough. Let's just do that in 0.8.0 then?

@fson
Copy link
Contributor Author

fson commented Nov 20, 2016

Yeah, I can see if removing bundled deps helps. If not, we can revisit the plugin option.

@fson fson closed this Nov 20, 2016
@mmazzarolo
Copy link

mmazzarolo commented Feb 6, 2017

@gaearon @fson I'm still a bit interested in this plugin, did you abandon the idea?
I'm implementing the same create-react-app config in our react-native project but I'd like to keep the devDependencies lighter instead of having 1 config and 4 plugins.
Thank you in advance guys and keep up with the good work! 👍

@mmazzarolo
Copy link

mmazzarolo commented Feb 7, 2017

@gaearon @fson I created eslint-plugin-mostaza-react which is basically the same plugin you created in this pull request.
The only difference is that I used mostaza-react instead of react-app as a prefix because I didn't want to steal the npm package name.
I tested the plugin on both a react and a react-native app and it works correctly.
As you can see "eslint-config-react-app" is a dependency: this package imports the rules directly from it.
Let me know if I can create eslint-plugin-react-app or if you'd prefer to keep the npm name for this project 🍹

@gaearon
Copy link
Contributor

gaearon commented Feb 7, 2017

@mmazzarolo This doesn't solve the IDE problem though, does it?

@mmazzarolo
Copy link

mmazzarolo commented Feb 7, 2017

@gaearon thanks for the fast reply.
I'm not aware of any IDE problem 🤔
If you're talking about the editor issue of the first post: I'm on VSCode and it works perfectly.
It seems that the issue of the first post was caused by shipping the plugin with CRA itself though (feel free to correct me if I'm wrong), while I'm discussing about an external plugin.

@gaearon
Copy link
Contributor

gaearon commented Feb 8, 2017

The original motivation was to solve an IDE problem on npm 2 (which we might not support soon anyway I guess). If you have another use case for this could you file another issue describing it and linking to this PR?

@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
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.

4 participants