IanVS / eslint-nibble

Ease into ESLint, by fixing one rule at a time
MIT License
787 stars 29 forks source link

Move eslint to a peer dependency #52

Closed IanVS closed 6 years ago

IanVS commented 6 years ago

Fixes #40 Closes #50

This package has always bundled eslint as a dependency because I wanted it to be easy to get up and running. The more-proper thing to do is to specify eslint as a peer dependency. Even though peer dependencies can be a bit confusing, I think it's likely that users of this package will generally already have eslint installed, and it makes a lot of sense to use their version, instead of bundling our own.

IanVS commented 6 years ago

I took eslint back out of dependencies for a few reasons:

Hopefully the readme is clear enough for people to know they need eslint to be installed. And if they try to install and run eslint-nibble without having eslint, they'll get errors that pretty clearly tell them they need to install eslint.

All that to say, I'm going to stick with just peerDependencies for now, and if I get a bunch of issues from confused users, I'll re-evaluate.

ljharb commented 6 years ago

Up to you; we do it in almost every package we author at airbnb - it works perfectly, as long as any peerDep range always has an identical range in either deps, or devDeps.

If yarn doesn't work with it, I'd file a bug on yarn, but it's fair not to do it if yarn is something that matters to you for some reason.

IanVS commented 6 years ago

Thanks for the feedback. Do you have any particular reasons that you think including eslint in dependencies would be an advantage?

The fact that it doesn't work well on yarn is a bummer, but isn't a showstopper on its own. And I could open an issue with them, but it seems similar to https://github.com/yarnpkg/yarn/issues/3951, and I'd wager my issue would be a wontfix for them as well (see also https://github.com/yarnpkg/yarn/issues/4125#issuecomment-322475791).

Aside from all that, using plain peer dependency has an advantage in simplicity. I'll be able to say, "'eslint-nibble' uses the version of eslint you have already installed in your project." without the complexity of figuring out whether this package installed its own version or not.

ljharb commented 6 years ago

npm already handles that complexity - by adding it as a dep, someone without eslint installed at all can use your package without any additional work, but for someone who already has a compatible eslint installed, eslint-nibble will happily use that one.

In the event of someone having eslint 3, or eslint 6, npm ls or npm ls -g will print out an error, and they'll hopefully be able to address it.

IanVS commented 6 years ago

someone without eslint installed at all can use your package without any additional work

That's true, but they'd also need to create an eslint config file for this tool to do any good, and for that, they'd need to know what version of eslint would be used, since config files have changed between many of the major versions. Yes they could use npm ls eslint to figure it out, but now we've taken something that was supposed to be easy, and made it more complicated again.

I'm sure that using the hybrid approach you suggested works great in a lot of cases. I'm just not convinced it's the right thing for this project. But thanks again for the suggestion and discussion!