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

[BUG] npm install sometimes removes indirect dependencies if a parent node was deleted from the lockfile #7746

Open
2 tasks done
TrevorBurnham opened this issue Aug 22, 2024 · 2 comments · May be fixed by #7752
Open
2 tasks done
Labels
Bug thing that needs fixing Needs Triage needs review for next steps

Comments

@TrevorBurnham
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

This issue exists in the latest npm version

  • I am using the latest npm

Current Behavior

My team sometimes deletes nodes from package-lock.json as specific package versions are deleted from our internal registry. Our expectation is that npm install should then install the latest version of that package that satisfies the requirements in package.json, without changing indirect dependencies unnecessarily.

However, we're seeing an issue where npm install sometimes deletes an indirect dependency of the deleted node, even as it replaces that node. We have to run npm install a second time in order to restore the indirect dependency.

This issue only seems to occur when there's a different version of the indirect dependency installed.

Expected Behavior

We expect package-lock.json to always be in a consistent state after an npm install, with all dependencies satisfied.

Steps To Reproduce

I've created a CodeSandbox: https://codesandbox.io/p/devbox/quirky-rain-rv9lkl You can create a new project in the same state by running npm install glob@7.1.6 && npm install minimatch@4.2.3 && npm install mocha@10.7.3.

Once you have that project set up, the steps to replicate the bug are:

  1. Delete the "node_modules/mocha" node from package-lock.json.
  2. Run npm install.
  3. Check the diff and see that npm restored the "node_modules/mocha" node, but removed the "node_modules/mocha/node_modules/brace-expansion" node. That directory has also been deleted from node_modules. This means that mocha's indirect dependency on brace-expansion@^2.0.1 (by way of its dependency on minimatch@^5.1.6) is unsatisfied; mocha would instead use brace-expansion@1.1.11, which is installed at the root of node_modules.
  4. Run npm install again and observe that "node_modules/mocha/node_modules/brace-expansion" has been restored in both package-lock.json and node_modules.

Environment

I've observed this issue in npm v8, v9, and v10.

  • npm: 10.8.2
  • Node.js: 20.16.0
  • OS Name: macOS 14.6.1
  • System Model Name: M1 MacBook Pro
  • npm config:
; node bin location = /Users/trevorburnham/.asdf/installs/nodejs/20.16.0/bin/node
; node version = v20.16.0
; npm local prefix = /Users/trevorburnham/Code/lockfile-with-missing-parent-testcase
; npm version = 10.8.2
; cwd = /Users/trevorburnham/Code/lockfile-with-missing-parent-testcase
; HOME = /Users/trevorburnham
@TrevorBurnham TrevorBurnham added Bug thing that needs fixing Needs Triage needs review for next steps labels Aug 22, 2024
TrevorBurnham pushed a commit to TrevorBurnham/cli that referenced this issue Aug 23, 2024
This test case demonstrates npm#7746.
Currently, it fails. Note that it passes if reify is called twice.
@TrevorBurnham
Copy link
Contributor Author

I've put together an arborist test case to demonstrate the issue: https://github.com/TrevorBurnham/cli/tree/7746-bug-test-case

You can run the test case on that branch with this command:

npm test -w @npmcli/arborist -- test/arborist/reify.js --no-cov -g="parent"

Notably, the test passes if you perform reify twice: The second reify restores the indirect dependency that the first reify deleted,

TrevorBurnham pushed a commit to TrevorBurnham/cli that referenced this issue Aug 25, 2024
This addresses an edge case where a dep could be placed in the tree
with unsatisfied indirect dependencies (see test case).
@TrevorBurnham
Copy link
Contributor Author

I've submitted a PR that appears to address this issue (at least as far as the test case is concerned): #7752

I'd welcome alternative fix suggestions. I'm new to this codebase, but from what I can tell, here's what's happening when building the ideal tree in the test case:

  1. The dependency on the deleted node is identified as a problem edge and the information needed to recreate the node is fetched from the registry.
  2. When the new node is placed on its parent, that edge is now considered satisfied. And since there are already nodes for all of its direct dependencies, all of its edges out are also considered satisfied (i.e. it has no problem edges).

So the problem is that no checks are performed for indirect dependencies of the placed dep. The PR addresses that by adding the placed dep's children to the deps queue, so that they get checked for missing deps.

I'm sure there's a more elegant solution that I'm missing! I'd love to hear any thoughts.

TrevorBurnham pushed a commit to TrevorBurnham/cli that referenced this issue Aug 25, 2024
This addresses an edge case where a dep could be placed in the tree
with unsatisfied indirect dependencies (see test case).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing Needs Triage needs review for next steps
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant