What to do about unlisted dependencies?

[isaacs]

This is a call for an RFC to address a problem. Trying out a new thing,
using issues here. Sort of a request for comments on a potential request
for comment :)

Prompted by discussion in #43

Situation

Given this stated dependency graph:

foo -> bar
bar -> baz

Where the code in foo calls require('baz'), but does not list it as a
dependency.

In npm from version 3 onwards, the resulting install tree will look like:

foo
+-- bar
+-- baz

so require('baz') in foo's code will resolve to the baz being pulled
in by bar.

If we implement #43, then this has the
potential to get slightly worse, because peerDependencies will always
be installed where the dependent can reach them.

In other words, while this install tree would be valid for regular
dependencies:

foo
+-- bar
    +-- baz

if bar has a peer dependency on baz, then it must be installed where
foo can resolve it, thus enforcing deduplication which makes unlisted
dependencies possible.

Furthermore, pnpm installs using a different approach, which does not
support unlisted dependencies. In pnpm's install layout, the tree would
look something like (simplified):

node_modules
  foo -> symlink to /node_modules/foo@1.2.3/node_modules/foo
  foo@1.2.3
    node_modules
      foo (actual code)
      bar -> symlink to /node_modules/bar@1.2.3/node_modules/bar
  bar@1.2.3
    node_modules
      bar (actual code)
      baz -> symlink to /node_modules/baz@1.2.3/node_modules/baz
  baz@1.2.3
    node_modules
      baz (actual code)

In a pnpm installation, the code in foo lives in a node_modules folder
where bar is reachable, but baz is not reachable. Thus, the unlisted
foo -> baz dependency will cause foo to not function properly. (Note:
pnpm's --shamefully-flatten configuration will create a tree where
unlisted deps are resolvable, but this is not the default.)

Beyond pnpm support, this presents some other problems.

• bar may upgrade to a new breaking change to baz, but in such a way
  that it does not require a breaking change in bar. (Ie, a change to
  API that bar abstracts away.) This can break foo's usage of baz
  in unexpected ways without warning.
• Analysis of data about dependencies is thwarted by unlisted deps.
  baz's author may not realize that foo is a user, and this information
  can be valuable for the authors of both foo and baz.
• Fixing this problem is usually merely a matter of listing the dependency.
  However, authors may be resistant to making a code change for something
  that "already works fine".

Options

The options fall into three general categories:

• Do nothing
• Detect unlisted deps
• Design a package tree that breaks unlisted deps

Do nothing. (Aka: "This is fine." 🔥 ☕ 🐶 🔥)

We can accept that unlisted deps are just a fact of life, and let the
community continue to deal with it. Document listing your deps as a best
practice, try to get people to learn why it's bad, etc. But otherwise, do
nothing.

One argument for this would be that unlisted deps aren't a particularly
severe hazard. I think that's a hard case to make, given the issues above.

Another argument is that this is a problem for pnpm, but not one that faces
npm, so it's not ours to fix. Again, hard case to make, because it's a
problem for pnpm that npm creates, so we probably ought to fix it.

• Drawbacks doesn't solve problem.
• Benefits extremely trivial to implement.

Detection

We can attempt to detect unlisted deps either at install time, run time,
via an explicit command (like npm doctor or npm ls), or asynchronously
on the registry.

Having been detected, we can provide an automatic mechanism to fix the
problem (run 'npm install foo@version' to remove this warning, or even
npm autoinstall).

Finally, once detected, unlisted dependencies cna be either a warning, or
crash the process.

Install Time

Detecting the use of require() in installed code is very challenging.
Users can have code in any file in a project which calls require() (or
import), which may even modify the module system, be transpiled, etc.

Additionally, this is difficult when a program calls require() with a
variable rather than a string literal.

• Drawbacks either incomplete, or line-item AI. warnings are easy to
  ignore.
• Benefits alerts users to the problem without run-time breakage

Run Time

After npm version 8, we can instrument the tink module loader to detect
read access to a file in a package that is unlisted, but appears in the
package-lock.

We'd probably not want to do this in production (ie, when
NODE_ENV=production or --production config flag set). And, it's more
likely to be ignored if warnings are for dependencies deep in the tree
(since the user seeing the warning can't But, if warnings were printed
whenever users ran tests, they'd be in a position to fix the error right
there (especially if it caused a test failure).

• Drawbacks depends on npm v8, possible to cause run-time disruption
• Benefits fairly straightforward once tink is integrated, unlikely to
  be ignored by users.

Detect Explicitly

When the user runs npm ls or npm doctor, we could scan for unlisted
dependencies. This would have all of the same drawbacks as detecting at
install time (ie, scanning code), but without automatically being run on a
regular basis. Comparing behavior of npm audit vs the quick-audits that
automatically run on npm install, it would seem this is not an ideal
approach.

If we explicitly detected unlisted deps on npm test (and only then), then
this would have most of the benefits of detecting at run-time, without
production run time disruption.

Detect On The Registry

We scan packages on the registry, and nag users with emails or something
when their package references a dependency that's not listed.

• Drawbacks line-item AI (but at least, it's an AI that already partly
  exists, for detecting malware), users hate nag emails, additional
  server-side work with little benefit to the registry provider, depends on
  a single registry provider
• Advantages extremely trivial for CLI team

Warning on Unlisted Deps

Print a warning when an unlisted dep is found. Treat it as a reason to
complain, but not a reason to crash.

• Drawbacks easy to ignore, annoying (see the "why are you nagging me
  if this works fine??" response that we get from nags about missing
  fields in package.json.)
• Advantages minimize run-time disruption

Error on Unlisted Deps

Treat it as an actual error, some or all of the time.

This is likely justifiable in an explicit build- or dev-time situation like
npm test or npm doctor, but probably too aggressive to do in npm start.

If we detect unlisted deps in install, then we'd also have to decide
whether this is an offense worthy of canceling and rolling back the build,
or merely exit non-zero after the install completes.

• Drawbacks even more annoying, possibility of run-time disruption
• Advantages harder to ignore

Break Functionality with Package Installation Layout

We could adopt a package tree layout approach somewhat like pnpm, such
that unlisted dependencies would no longer function.

While this would remove the "it works, why bother?" objection to fixing
it, it would also be extremely costly to implement, since we'd still need
to maintain support for the classic "deduped" package layout approach.

Additionally, since many packages using pnpm already set the
shamefully-flatten config value, it would appear that doing this won't
actually make users change their behavior. It's possible that npm
dropping support for unlisted deps will cause them to take more notice, but
it's also likely that this will be viewed as a bug and routed around, or
that users will be so disrupted we'll be compelled to add a workaround,
thus defeating the point of this approach.

[isaacs]

cc @zkochan @arcanis

[zkochan]

Thanks for considering this.

To the problems of flat node_modules, I would also add that a subdep can actually disappear. The bar can stop relying on baz and baz will disappear altogether from node_modules. (I mentioned this in pnpm’s strictness helps to avoid silly bugs.)

Also, a relatively easy way to fix this on the npm side in v7 would be making the global-style config true by default.

[isaacs]

Also, a relatively easy way to fix this on the npm side in v7 would be making the global-style config true by default.

Ah! Yes, that is true. It'd be sort of a less aggressive form of the "break package layout" approach.

The hazard there might be that deduplication is valuable for people who build code for the front-end. You wouldn't want to have 10 copies of a shared frontend asset in your bundle. In fact, there have even been some suggestions to use a bower-style "deduplicate everything, manually resolve conflicts" type of approach.

[zkochan]

But I also see value in things like npm doctor or detecting these issues when running tests. And you could search not only for requires of unlisted deps but also requires of dev deps. I created a tool that does this package-preview. package-preview basically packs the package, installs it as a dependency in some external folder and runs the tests on that "semi-published" package

[ljharb]

Note that eslint-plugin-import, included in the Airbnb eslint config, prevents this problem.

[zkochan]

There's also a simple solution to fix this using symlinks.

1. Make a flat node_modules in a different folder. For instance, in .flat/node_modules.
2. Only symlink the direct dependencies from .flat/node_modules to node_modules.

As a result, dependencies will have access to all unlisted deps (via .flat/node_modules) but application code will only have access to the listed dependencies (because only they will be symlinked into node_modules).

I am thinking about implementing such structure for pnpm (see pnpm/pnpm#1998).

The only con of this solution is that it won't work with node's --preserve-symlinks flag.

[isaacs]

There's also a simple solution to fix this using symlinks.

Well, that is a simple solution to the implementation, assuming we want to take on the pain of making a breaking change in this area. But I do like that it puts the burden only on the developer using unlisted deps, rather than on users.

The only con of this solution is that it won't work with node's --preserve-symlinks flag.

Yeah, but lots of stuff doesn't work with that.

I suppose one way to thread the needle would be:

node_modules/
  direct-deps @-> ./.flat/node_modules/direct-deps
  .flat/
    node_modules/
      direct-deps
        node_modules/
          meta-deps @-> ../../meta-deps # only if deduped, otherwise a "real" folder"
      meta-deps

It'd work in preserve-symlinks mode, because it'd just look like the npm@2 style:

node_modules/
  direct-deps/
    node_modules/
      meta-deps/

My main objection to something like this is that symbolic links are kind of a pita, especially for Windows support. We'd potentially break some use cases with ELOOP cycles, as well, if we're not careful. Of course, pnpm has solved all these problems, but it'd basically mean adopting that kind of approach and rewriting a lot of code in a different way.

[isaacs]

Another possible approach:

node_modules/
  direct-dep1
  direct-dep2
  node_modules/
    meta-dep1
    meta-dep2

All direct dependencies of the current project go in node_modules. Any other dependencies start in ./node_modules/node_modules/.

If direct-dep1 has an unlisted dep on meta-dep2, it'll still load, because it's in a parent's node_modules folder.

This would be relatively easy to reify by just changing the Node.path field based on whether it's a root dependency. But reading the tree would have to be a bit more involved, since it'll have to handle cases like this:

node_modules/
  foo
  node_modules/
    foo

In that case, foo loaded by the root project will be different from the foo loaded by any dependencies. The data model will have to be updated to support shadowing a dependency at effectively the same level in the tree. nm/nm/foo is sort of like a child of the root node, but the root node can't resolve it. It'd almost make sense to have a special "shadow" node that has children and a parent, but doesn't participate in dep resolution directly.

[ljharb]

That seems like a great solution, but would it require node resolver changes? Also, can you have a package named “node_modules” already, including a manually created folder?

[isaacs]

Nope! that's what makes it a good solution :)

[ExE-Boss]

pnpm v4 does a similar thing when hoisting is enabled, but puts the nested node_modules folder inside the .pnpm folder:

node_modules/
	foo → .pnpm/registry.npmjs.com/foo/v1.0.0/node_modules/foo/
	.pnpm/
		# This directory is used so that dependencies with undeclared
		# sub‑depencencies don't break when using `pnpm`
		node_modules/
			foo → ../registry.npmjs.com/foo/v1.0.0/node_modules/foo/
			bar → ../registry.npmjs.com/bar/v1.0.0/node_modules/bar/
		registry.npmjs.com/
			foo/v1.0.0/node_modules/
				foo/
					index.js
					package.json
				bar → ../../../bar/v1.0.0/node_modules/bar/
			bar/v1.0.0/node_modules/
				bar/
					index.js
					package.json