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

CRA and Airbnb require conflicting versions of eslint-plugin-jsx-a11y #3540

Closed
nickmccurdy opened this issue Dec 2, 2017 · 11 comments
Closed

Comments

@nickmccurdy
Copy link
Contributor

nickmccurdy commented Dec 2, 2017

This makes it difficult to use CRA with eslint-config-airbnb@16, which is very popular with React development.

Is this a bug report?

Yes.

Can you also reproduce the problem with npm 4.x?

Yes.

Which terms did you search for in User Guide?

  • Airbnb
  • accessibility
  • a11y

Environment

  1. node -v: 8.9.1
  2. npm -v: 5.6.0
  3. npm ls react-scripts (if you haven’t ejected): 1.0.17

Then, specify:

  1. Operating system: macOS 10.13.1

Steps to Reproduce

  1. Install create-react-app.
  2. Install eslint-config-airbnb@16.

Expected Behavior

CRA should support eslint-plugin-jsx-a11y@16.

Actual Behavior

These errors are mutually exclusive but I'm including them both for convenience and debugging purposes.

npm WARN eslint-config-airbnb@16.1.0 requires a peer of eslint-plugin-jsx-a11y@^6.0.2 but none is installed. You must install peer dependencies yourself.
npm WARN eslint-config-react-app@2.0.1 requires a peer of eslint-plugin-jsx-a11y@^5.1.1 but none is installed. You must install peer dependencies yourself.
@nickmccurdy
Copy link
Contributor Author

This might be related to #3418.

@gaearon
Copy link
Contributor

gaearon commented Dec 2, 2017

We don’t officially support using ESLint configs other than the default one without ejecting. Depending on whether our versions are in sync, it may or may not work, but we won’t commit to always supporting it. In fact I’d say we disagree with some decisions in the Airbnb config and think they’re not great universal rules for people developing React apps.

Are there particular reasons you need another config? We’ve tried to include rules that would be valuable for everyone, but exclude any styling rules (so you can use tools like Prettier to automatically format your code).

@gaearon gaearon closed this as completed Dec 2, 2017
@gaearon
Copy link
Contributor

gaearon commented Dec 2, 2017

If there are specific rules that you feel are missing in our default config, please raise a separate issue and we will discuss adding those.

@gaearon
Copy link
Contributor

gaearon commented Dec 2, 2017

(That said you can eject and use any config if that’s your strong preference.)

@nickmccurdy
Copy link
Contributor Author

Crossposting my comment on #3418:

I need eslint in all my projects for CI purposes and I would prefer not to be able to run a different version of it or at least have a different config using the same version of eslint. In my opinion, a boilerplate should not limit my ability to use a popular tool, and if there's anything I can do to help I'd be happy to contribute.

@nickmccurdy
Copy link
Contributor Author

nickmccurdy commented Dec 4, 2017

My issue is not with CRA's default eslint config (in fact I think it's great, I just want stricter configs for my projects that are more consistent with my other open source projects that already use shared eslint configs). I personally see CRA as a build tool that shouldn't limit my ability to use other JS ecosystem tools (except for Jest since it's still pretty configurable in CRA and it is sort of a build tool), though I understand there are major debugging benefits to including eslint by default and what I'm asking for may be too difficult to implement.

I know this involves more of a maintenance burden, but do you think it would be worth it to use some sort of workaround for eslint users (alternatively I would fork/eject my own shared config based off CRA or use a different tool)? For example, eslint could simply be inlined as a build-time dependency of CRA, which IMO would make the problem vanish (it would not be in the package.json, which shouldn't be an issue anyway since the eslint config is meant to be used internally, not with separate instances of eslint). Also I know that CRA has been considering plugin support, theoretically could the internal eslint config be moved to an optional plugin that is enabled by default in the future?

Also I appreciate the advocation of Prettier and while it does make sense for many projects to only use Prettier for styling and only use eslint to catch bugs (like with CRA's default config), there are many useful eslint rules that are outside of Prettier's domain of simply reformatting from the AST while being too opinionated to be moved into CRA's default configs. For example, the eslint-plugin-import/import rule with absolute-first enforces that absolute imports are always before relative imports. This is something that's really helpful for making larger codebases consistent, but it would be too restrictive to add this to the default eslint config and it's not strictly a syntax issue so it can't be handled by Prettier (at least as far as I'm aware).

@Timer
Copy link
Contributor

Timer commented Dec 4, 2017

a boilerplate should not limit my ability to use a popular tool

We're not a boilerplate. 😄
If you'd like to treat react-scripts as a "boilerplate", that involves ejecting -- at which point you can tune things however you please.

I can't use eslint with CRA even if I execute it separately with a different config?

Sure you can! Due to eslint limitations [more on that below], you simply can't install it side-by-side to react-scripts; try a directory higher.

I know this involves more of a maintenance burden, but do you think it would be worth it to use some sort of workaround for eslint users (alternatively I would fork/eject my own shared config based off CRA or use a different tool)?
-snip-

We're open to making this work by default [installing eslint next to react-scripts], but we don't have the time to work on that right now. I'd be open to a pull request for that.

To be clear: in no way should any custom eslint configuration or rules have an effect on our development or build process; it simply shouldn't break it / prevent the user from having their own lint step.

@gaearon
Copy link
Contributor

gaearon commented Dec 4, 2017

For example, the eslint-plugin-import/import rule with absolute-first enforces that absolute imports are always before relative imports. This is something that's really helpful for making larger codebases consistent, but it would be too restrictive to add this to the default eslint config and it's not strictly a syntax issue so it can't be handled by Prettier (at least as far as I'm aware).

You can add such individual rules to your .eslintrc and then run eslint yourself manually. Of course, this wouldn't work if any plugin versions are incompatible with the ones we use, but I don't see a solution here—we can support only one major version at a time. This is why including other presets (like Airbnb) is more challenging because now there are many plugins which all have to match in versions.

EugeneAlexeenko added a commit to EugeneAlexeenko/weather that referenced this issue Dec 14, 2017
@mwygoda
Copy link

mwygoda commented Jan 18, 2018

@gaearon Could you please tell with which of Airbnb lint rules you disagree?

@gaearon
Copy link
Contributor

gaearon commented Jan 18, 2018

At the very least, I disagree with inclusion of the styling rules. I think our users would be better served by tools like Prettier that format the code automatically. I’ve seen beginners learning React spending minutes trying to correct all the irrelevant style nits when they’re just trying to learn some concept. Airbnb config also marks those as “errors” so beginners genuinely think they’re making a mistake somewhere. For comparison, the default config we ship has no style rules.

Similarly, Airbnb config considers code uniformity very important so it has rules like “prefer stateless components when not using state/lifecycles”, “use arrows everywhere you can” etc. In my observation this isn’t useful to beginners who copy and paste existing code and expect it to work. It also creates faux “best practices” and dogma in the ecosystem that we don’t necessarily agree with. And even seasoned professionals don’t like to convert functions to classes back and forth because the state ownership is still unclear in the initial project stages. I think developers should be trusted to make such decisions themselves.

My observation is that once beginners learn that those rules aren’t really as important as they were lead to believe by blogs/examples/perceived strictness, they start ignoring them altogether, at least during active development. And that’s a shame because ESLint often finds actual mistakes in the code, but being trained by overly restrictive configs, users gloss over them.

I know that if we allowed easy customization of the ESLint config that is used for our default lint reporting on npm start, many open source examples using CRA will be pressured into adopting Airbnb config because it’s popular and many experienced folks feel that it validates their coding style. I think that overall even allowing a restrictive config like this to be used in CRA terminal/console lint output will hurt the beginner experience with those examples, and ultimately it hurts React learning and adoption.

I hope this explains it somewhat.

That said, the a11y plugin version has been bumped in the latest react-scripts@2.x alpha so you can try it: #3815.

@mwygoda
Copy link

mwygoda commented Jan 19, 2018

Thanks for response! Totally agree with not beginner friendly part (especially styling rules). Forcing user to use ALWAYS(when state/lifecycles not requried) functional components instead of class based could be pain in the neck as well. Anyway for a bit more experienced user could be starting point for adapting some of rules and fit to project code style. Ofc solution could be adding some rules to less restricted config. Anyway 100% agree it shouldn't be a default set of rules.

Ninja-Recai added a commit to Ninja-Recai/event-registration-form that referenced this issue Sep 5, 2018
- Give up code style linting in favor of CRA's linting
methods. For the cause visit
(facebook/create-react-app#3540)
@lock lock bot locked and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants