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

Be more lax about peer dependencies in eslint-config-react-app #1110

Closed
gustavnikolaj opened this issue Nov 29, 2016 · 9 comments
Closed

Be more lax about peer dependencies in eslint-config-react-app #1110

gustavnikolaj opened this issue Nov 29, 2016 · 9 comments
Milestone

Comments

@gustavnikolaj
Copy link

When using CRA with an additional linter I hit problems with matching peer dependency versions with eslint-config-react-app. It has absolute requirements for its peer dependencies.

{
  "peerDependencies": {
    "babel-eslint": "7.0.0",
    "eslint": "3.8.1",
    "eslint-plugin-flowtype": "2.21.0",
    "eslint-plugin-import": "2.0.1",
    "eslint-plugin-jsx-a11y": "2.2.3",
    "eslint-plugin-react": "6.4.1"
  }
}

(https://github.com/facebookincubator/create-react-app/blob/a5f760bab9b1876df13ac4345a4b42655fec46fa/packages/eslint-config-react-app/package.json)

Specifically babel-eslint, eslint, and eslint-plugin-react is hard not to clash with. I had another eslint-config that listed all three of them with incompatible versions. As an example it listed eslint as ^3.11.0.

Had eslint-config-react-app used the caret-style dependencies for it's peer dependencies, it would have worked out fine.

In general, I understand why you might want to absolutify your linter dependencies - there's nothing more annoying than lint errors showing up on CI that you didn't catch locally. But in this case, where it's very common to add another linter setup I think it might be worth it, to be more lax about the requirements.

For my case, pinning the versions listed in peerDeps of eslint-config-react-app got rid of the peer dependency warnings, after I had downgraded the requirements in the other eslint-config I used.

Would you be open to widen the peer dependency version ranges in eslint-config-react-app?

@gustavnikolaj gustavnikolaj changed the title Be more lax about peer dependencies in eslint-config- Be more lax about peer dependencies in eslint-config-react-app Nov 29, 2016
@gaearon
Copy link
Contributor

gaearon commented Nov 29, 2016

@fson Any thoughts?

@fson
Copy link
Contributor

fson commented Nov 29, 2016

In the past I've had ESLint plugins misbehave after upgrading to a new minor version, and I don't want this to happen in CRA. However, we can still keep the strict versions for these dependencies in react-scripts, so loosening the requirements for peer dependencies will only affect people using eslint-config-react-app directly. The risks are also diminished by using yarn.lock or npm-shrinkwrap.json, which advanced users should be doing already.

I think it's probably worth doing for compatibility with other configurations. We can also keep the exact versions pinned in the installation command we have in the readme, so we'll still have a "sanctioned" set of versions all the time that we have tested.

@gustavnikolaj
Copy link
Author

gustavnikolaj commented Nov 29, 2016

@fson I completely understand your stance on this matter, and I agree wholeheartedly. But I think that it is better controlled from the projects themselves and not by third party library code. In my opinion, both CRA and eslint-config-react-app fall into the latter category.

As a user of CRA I'd expect that I could just run npm install --save-dev standard and add a lint script to my package.json and be up and running. That will not work without triggering peerDependency warnings if standard relies on a package that is too newer than one defined in eslint-config-react-app. As of writing, this will actually work, as the most recent version of eslint-config-standard has a peer dependency on eslint >=3.8.1. If that was changed to >=3.8.2 we would start seeing the issue I described.

Most of the sharable eslint configurations that I have seen are using peerDependencies with either caret or greater-than-or-equal to, to avoid dependency hell. A few examples:

Doing the same here will allow people to get off the ground easily, with no friction, but still allowing them to freeze packages using shrinkwrap / yarn or manually adding absolute versions for the important packages.

@fson
Copy link
Contributor

fson commented Nov 29, 2016

That's a good point. My only concern is that a breaking change in an ESLint plugin is going to be published as a minor version and it's going to break new projects created with create-react-app.

Is there any way to be strict in what we install automatically when you run CRA, but at the same time allow users to upgrade if they need to? Should we start bundling a yarn.lock file in our project template?

@gustavnikolaj
Copy link
Author

My only concern is that a breaking change in an ESLint plugin is going to be published as a minor version and it's going to break new projects created with create-react-app.

Is there any way to be strict in what we install automatically when you run CRA, but at the same time allow users to upgrade if they need to? Should we start bundling a yarn.lock file in our project template?

I'm not aware of any way to change how the version ranges are interpreted by npm. Unless I've got it all wrong, it will install the latest package that satisfies your spec.

As I see it, breaking changes can primarily be introduced into eslint-config-react-app in two ways:

  1. A preset adds an additional rule.
  2. A rule changes behavior.

One way to mitigate some of the problem is to not depend on other eslint-config-* packages or presets, but instead list the rules directly in eslint-config-react-app. That way, it's only if a rule changes behavior or disappears completely from a given plugin.

That seems less likely to happen, and it does constrain our vulnerability :-)

@fson
Copy link
Contributor

fson commented Nov 30, 2016

We already list all the rules in eslint-config-react-app. But sometimes bugs are introduced in the plugins or ESLint itself, or they don't work with a certain version of babel-eslint etc.

@gustavnikolaj
Copy link
Author

Okay... In that case, I don't think there's any thing we can do to mitigate the problem. It's an inherent problem to the node eco system and how npm works.

The open version ranges allow people to push out new versions that fix those bugs, without requiring a new release of CRA or other projects that depend on the given package.


My personal opinion is that the current version ranges are too conservative for a module, which is meant to be plugged into other projects.

As the eslint config is style-agnostic by choice, users are pretty much required to bring their own linter - and eslint being the defacto standard linter the absolute peer dependency versions will cause additional friction.

I would be more okay with this strategy if CRA also bundled an opinionated linter. Then users wouldn't be required to bring their own, and the problem would be irrelevant. But that seems to be a major change in the direction of the package. (Just for the record, I don't think that's a good idea - just trying to make a point wrt. different expectations for different circumstances).


I realize that this is a matter of personal opinion and preference (religion? 😄) so I'll get out of your way now that I presented my view. I trust that you'll find the optimal tradeoff for this project. :-)

@EnoahNetzach
Copy link
Contributor

EnoahNetzach commented Dec 5, 2016

Another way possible is to give an explicit range, so that the minimum is needed for CRA to work, and the maximum is granted to work well without having bugs.

This, on one other hand, is quite a lot of work on the maintainers' side, but on the other hand grants more stability to those who have to enforce a stricter lint.

@gaearon
Copy link
Contributor

gaearon commented Dec 7, 2016

Fixed in the latest version of the preset.

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