search

npm / cli

the package manager for JavaScript
https://docs.npmjs.com/cli/
Other
8.46k stars 3.15k forks source link

[BUG] Dependency hoisting in workspaces not working, depending on workspace name #4512

Open loilo opened 2 years ago

loilo commented 2 years ago

Is there an existing issue for this?

This issue exists in the latest npm version

Current Behavior

If a workspace folder has the same name as one of its direct dependencies, that dependency will not be hoisted to the project root on install.

Note that behavior only applies for top-level dependencies. Nested dependencies will still be hoisted properly.

Expected Behavior

The dependency should be hoisted to the root.

I'm not entirely sure whether this is actually a bug or a constraint due to the Node module resolution algorithm or something similar. However, I have not found any official or inofficial documentation of this behavior. If the behavior is by design, this issue could at least serve as an anchor for other users who run into the same problem.

Steps To Reproduce

Follow the steps in the reproduction repository I made: loilo/repro-npm-workspaces-hoisting.

Environment

prefix = "/usr/local"

; "user" config from /Users/florian/.npmrc

//localhost:4873/:_authToken = (protected) //registry.npmjs.org/:_authToken = (protected) http://localhost:4873 = ""

; node bin location = /usr/local/Cellar/node/17.6.0_1/bin/node ; cwd = /Volumes/RAMDisk/repro-npm-workspaces-hoisting ; HOME = /Users/florian ; Run npm config ls -l to show all defaults.

nlf commented 2 years ago

this one looks to be an issue where we use the directory name of the workspace as an implicit package name if the package.json lacks a name field.

we effectively think your workspace is the actual jest, but since there's also no version field we determine that it isn't valid for any of the other requirements, so after we symlink the workspace as node_modules/jest we see the package is invalid for all of the other requirements, and since we can't hoist over the workspace, we instead install a new unhoisted jest in every workspace that requires it.

there's not really anything we can fix here, but it would be a great idea to call this out explicitly in the workspaces documentation because i can definitely see how that would be confusing!

ljharb commented 2 years ago

I wonder if there's room for a config value that avoids the implicit package name entirely? I'd basically never want that behavior; i'd always want npm to force all of my workspace packages to have an explicit name.

nlf commented 2 years ago

I wonder if there's room for a config value that avoids the implicit package name entirely? I'd basically never want that behavior; i'd always want npm to force all of my workspace packages to have an explicit name.

i don't see why not. it certainly creates situations that aren't obvious to debug. i'd almost even suggest that we log a warning for any workspace that doesn't specify a name whether you have the implicit name enabled or not. there's also potentially some similar work we could do to address workspaces that lack a version field.

loilo commented 2 years ago

Alright, thanks for the clarification! It didn't come to mind for me that package names could be required as we merely use workspaces for distributed declaration of dependencies.

I guess we leave this open untill this behavior is documented?

nlf commented 2 years ago

I guess we leave this open untill this behavior is documented?

that's the idea!