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

Consider removing all dependencies and not "relying" on npm flattening. #1

Closed
ryan-roemer opened this issue Jun 24, 2016 · 4 comments
Closed

Comments

@ryan-roemer
Copy link

ryan-roemer commented Jun 24, 2016

Absolutely fantastic post at https://blog.scottnonnenberg.com/eslint-part-1-exploration/ Scott!

One thing that caught my eye was: https://blog.scottnonnenberg.com/eslint-part-1-exploration/#one-install

I wanted to make it a single install, so my configuration includes ESLint and all needed plugins. And this means, given ESLint plugin resolution rules, that you need npm version 3 and its flat node_modules directory structure. It seems like a reasonable tradeoff. peerDependencies are messy.

You are most certainly correct that peerDependencies are messy. But a bigger issue is making sure to never ever allow folks to "rely" on npm to flatten a dependency to any reasonable location.

Let's look at our dependencies here:

  "dependencies": {
    "eslint": "2.11.1",
    "eslint-plugin-bdd": "2.1.0",
    "eslint-plugin-chai-expect": "1.1.1",
    "eslint-plugin-filenames": "1.0.0",
    "eslint-plugin-immutable": "1.0.0",
    "eslint-plugin-import": "1.8.0",
    "eslint-plugin-jsx-a11y": "1.2.3",
    "eslint-plugin-no-loops": "0.2.0",
    "eslint-plugin-react": "5.1.1",
    "eslint-plugin-security": "1.2.0",
    "@scottnonnenberg/eslint-plugin-thehelp": "0.3.2"
  },

So as a thought experiment, where can the [email protected] script end up as a top level dependency? the answer is any of:

<root>/node_modules/.bin/eslint
<root>/node_modules/@scottnonnenberg/eslint-plugin-thehelp/node_modules/.bin/eslint

The other deps are likely reliable due to the way node require resolution works. (Unless eslint requires them to be at <root>/node_modules/<right-here>. If so, then you similarly can't guarantee that with the structure here -- you need root project deps to guarantee root location).

But here's the next thing with the extra non-eslint dependencies: are all the uses of this package going to use all those plugins?

For instance, if I have a vanilla js / node project, I take the hit of unused plugins: eslint-plugin-react, eslint-plugin-immutable and other things I'm not using.

This is a long way of saying most of the projects I've seen and helped with either do use peerDependencies, or instead document, but don't enforce deps (like https://github.com/walmartlabs/eslint-config-defaults)

Thoughts?

@ryan-roemer ryan-roemer changed the title Consider removing all dependencies and not relying on npm flattening. Consider removing all dependencies and not "relying" on npm flattening. Jun 24, 2016
@scottnonnenberg
Copy link
Owner

Hey Ryan! Sorry for the delay - apparently I wasn't watching this repo back when you added this issue. It was the first PR that brought me here!

Size

First, thanks for this. It brought my attention to two far-too-large dependencies, which I PR'd:

The full set of module sizes shows that eslint-plugin-import and eslint-plugin-react are the largest, and then even post-PR eslint-plugin-jsx-a11y will be around 350k, so it's next on the list.

2.1M    node_modules/eslint
 28K    node_modules/eslint-import-resolver-node
132K    node_modules/eslint-plugin-bdd
 36K    node_modules/eslint-plugin-chai-expect
 52K    node_modules/eslint-plugin-filenames
 32K    node_modules/eslint-plugin-immutable
508K    node_modules/eslint-plugin-import
992K    node_modules/eslint-plugin-jsx-a11y
 32K    node_modules/eslint-plugin-no-loops
336K    node_modules/eslint-plugin-react
 84K    node_modules/eslint-plugin-security

So from a size perspective I could see some benefit to moving to a 'you should install these node modules manually' configuration.

Directory structure

But I'm less convinced about a 'non-flattened' situation causing major problems. There are only a few cases I'm aware of here:

  1. You're using an old pre-flattening version of npm
  2. You've specified your own version of one of our dependencies
  3. You're using a package manager other than npm

Do you know of other scenarios? Know of specific package managers to include in 3)?

Simplicity!

The reason I went with the current design was simplicity. You don't need to dig into the documentation to figure out what dependencies you need to install, which versions are compatible, etc. You just get what you need. The whole point is to be a comprehensive setup.

If you have a problem with one of the dependencies, you can install your own, npm will revert to a nested directory structure, and ESLint will pick up your top-level dependency instead. If you don't like the size of all the stuff installed, I recommend that people fork it!

@ryan-roemer
Copy link
Author

ryan-roemer commented Oct 15, 2016

The others scenario missing is "another dep has a nested dep on something in this package". Like a dep in package root with conflicting sub dep on eslint. The answer in the event of a failure / breakage outlined below, I think, shouldn't be to the consumer of this package to both diagnose why eslint versions are breaking and fixing it with a manual root dependency.

Or more succinctly, never rely on package location for non-root dependencies.

Non-root dependencies (aka things not explicitly in root 'package.json') can land anywhere in the dependency tree from their parent all the way down to the root. There are no guarantees or ways to ensure nested dependencies end up at the root, only luck at one point in time.

Put another way, npm3 can correctly end up with any structure that npm2 would given the right collection of nested deps

It may be unlikely other packages would bring in another waking for example, but you cannot guarantee it will always be at the root without a real root dependency.

@scottnonnenberg
Copy link
Owner

Yep, that is absolutely a missing 4), my mistake: "You install another module with a nested dependency overlapping with one of our dependencies"

I fully understand this: "npm3 can correctly end up with any structure that npm2 would given the right collection of nested deps." And I understood it when I wrote the announcement blog post, and when I released this package in the first place.

I know that I cannot guarantee that I work in scenarios 1) through 4), but I'm explicitly not optimizing for them. In so doing, I make the default case far simpler. You just install it. If you install other ESlint-related stuff, things can start to go haywire. At that point the user needs to educate themselves on this project's dependencies.

I'm fine deferring that education.

@ryan-roemer
Copy link
Author

ryan-roemer commented Oct 16, 2016

I'd encourage a loud and noisy note to your users about where eslint could be and why this module could break randomly at any unfortunate time a semver subdependency changes thing. Otherwise, things will just break and it wouldn't be obvious that npm3 not flattening is at issue.

And you could, for instance, link to https://www.npmjs.com/browse/depended/eslint (and this may be overbroad if include devdeps which wouldn't be at issue) with a note of "depending on any of these may break this package"...

I'm not sure most users can see just on inspection the approach in this project isn't guaranteed to have eslint in a specified place -- it's just luck that it's where the docs say it will be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants