[BUG] npm i produces different filesystem results before/after a specific time (likely related to a semi-related major version bump)

[ryanblock]

Is there an existing issue for this?

• I have searched the existing issues

Current Behavior

Currently all of our open source projects and many of our non-open source projects are unable to build because npm 7+ is reifying different file trees based on the publish event of an unrelated module. This meant that all of our builds that were working suddenly stopped working even though no dependencies changed on our side.

Here's a reduced test case repo with detailed explanation and steps to repro: https://github.com/ryanblock/npm-time-based-dependency-graph

Summary:

• This project has two dependencies: eslint@7 (which has many subdependencies) + @architect/eslint-config, which has 3 subdependencies:
  • eslint-plugin-filenames
  • eslint-plugin-fp
  • eslint-plugin-import
• When deps before 2021-10-09T20:46:00.000Z are installed:
  • The 3 eslint-plugin-$name dependencies above are installed flatly into node_modules/ (e.g. node_modules/eslint-plugin/filenames)
• When deps before 2021-10-09T20:47:00.000Z are installed (one minute later):
  • The 2 of 3 plugins are installed into node_modules/@architect/eslint-config/node_modules/ (specifically eslint-plugin-filenames, eslint-plugin-fp)
  • The 3rd plugin is still installed flatly into node_modules/
• eslint then runs, and cannot find eslint-plugin-filenames (nor eslint-plugin-fp, but it fails on the first plugin) because they are both installed in node_modules/@architect/eslint-config/node_modules/, away from the reach of Node's module resolution
• Note: eslint@8 DOES NOT appear in this dependency tree anywhere, however it was published at 2021-10-09T20:46:13.874Z
  • This implies that Arborist / npm is reifying module locations based on the existence of a package that is not even in use in this project

Expected Behavior

No matter when npm i is run in a project with identical dependencies, the filesystem results should be the same.

Steps To Reproduce

Steps to reproduce can be found at the following reduced test case repo: https://github.com/ryanblock/npm-time-based-dependency-graph

Environment

• OS: macOS 11.6
• Node: 16.10, 16.11
• npm: 7, 8 (does not reproduce in npm 6)

[lukekarrys]

I confirmed this happens in 7.20.3 but not 7.20.2 so this is likely a regression introduced by 97cb5ec.

[ryanblock]

This replicates for me in 7.20.0 as well, so perhaps that referenced commit is not the regression?
This does NOT replicate in 7.20.0 so I think you may be right:

[isaacs]

This is a really neat effect. Here's the fix.

Explanation:

We build the peer set for the eslint plugin, which has a peer dependency on something like eslint@* or eslint@>=3, resolving to eslint@8.0.0.

Then, we check to see where we can place the entire peer set, and in the process, we see that placing eslint@8.0.0 in the root is no good, because it conflicts with the root dependency on eslint@7.x, and call that a conflict. If we really had to place the dependency there, we'd just go ahead and do so, and warn about the conflict when it ended up actually having to be in conflict, because it's a transitive dependency.

But, because the eslint plugin is a regular dep of another dep in the tree, we say, "ok, no worries, we can just nest it to avoid the conflict".

Then we go to nest all the members in the peer set. But since eslint@7 is already present a level up in the tree, we don't bother actually placing the eslint@8.0.0, because it's not needed, and we prefer to dedupe peer deps, even if it means using an older version. (In other words, peer deps are always placed in --prefer-dedupe mode.)

So it gets nested to avoid a conflict that never manifests!

The solution in the gist is to only do the "does the target have an edge that conflicts with the dep we're trying to place" if there isn't already a current node in that position. If there is a current node there, we're going to do the work to see if we need to replace it anyway, so it'll still conflict when it needs to. If there isn't a node in that position, then this is the first thing we're trying to place there, and that conflict will definitely pose a problem.

This change causes a few snapshots to be a bit more aggressively deduped, but that's actually desirable.

Side note, relying on deduping behavior and the ability to require() transitive dependencies is extraordinarily brittle, as you're seeing here. In the short term:

• @architect/eslint-config should declare the eslint plugins it uses as peer dependencies, not regular dependencies, since it wants them to end up in the same level in the tree where it is.
• you as the root project could declare them as dev dependencies so that they get placed where you need them.
• eslint could take a module object as a plugin, instead of relying on being able to require() an arbitrary string it sees in a config file. (This isn't the first time this has broken due to incorrect assumptions made about npm's reification algorithm.)

This will never ever work in isolated-mode, so it's worth fixing sooner rather than later, regardless. npm's reification strategy is not public API surface. We guarantee that we'll satisfy the stated dependency contracts, and in this case, that's exactly what we're doing. But the specific folder paths should be treated as 100% black box -- you'll get the version of a package you depend on if you require() it, but there's no guarantee that your deps will ever be available to anyone else. What we have here is unnecessary nesting/duplication, which is suboptimal and should be fixed, but is not a "bug" in the sense of being incorrect behavior or violating any dependency contract.

[ryanblock]

Thanks for this thorough explanation @isaacs! This maps pretty well to what I imagined was happening under the hood.

Re. our need for peer deps: def agree, and @architect/eslint-config now does declare all those as peer-deps. For whatever it's worth, when I attempted that change yesterday after discovering this bug it still didn't improve the filesystem reification and the issue continued to replicate.

Additionally, I think eslint's means of loading plugins has changed with v8, and that seems to have also resolved our usage issue – we only remained broken because one of the dependencies of that config wasn't yet eslint@8 compatible (but it is now, thanks @ljharb!). Which is nice, but doesn't exactly help the lower-level npm side as described here.

I think the only thing I'd like to raise is that having determinism in folder path resolution is, imo, important. Although this particular case came about due to a confluence of circumstances, I'm sure plenty of instances like this exist out there (or may soon appear). And, for better or worse, it's also not unheard of for devs to reach directly into their modules' file paths (not that I'm telling you anything you don't already know).

If as of npm 7 dependency file paths can (no longer?) be treated as a deterministic outcome then I'd imagine that may erode trust in the system, no?

In any event, thank you for the escalation and resolution! 💕