FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(arborist): fix `peerOptional` dependency resolution in `buildIdealTree` by Saibamen · Pull Request #8981 · npm/cli · GitHub

/ cli Public

fix(arborist): fix peerOptional dependency resolution in buildIdealTree - #8981

Merged
owlstronaut merged 9 commits into
npm:latestfrom
Saibamen:fix_#8726
Feb 19, 2026
Merged

fix(arborist): fix peerOptional dependency resolution in buildIdealTree#8981
owlstronaut merged 9 commits into
npm:latestfrom
Saibamen:fix_#8726

Conversation

Saibamen commented Feb 16, 2026
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes #4859, fixes #5854, fixes #6787, fixes #8674, fixes #8726, fixes #8767, fixes #8777, fixes #8805

Tested on Windows & NodeJS v24.12.0 with package.json from #8726 (comment):

{
  "name": "testcase",
  "version": "1.0.0",
  "devDependencies": {
    "addons-linter": "6.13.0",
    "htmlhint": "1.1.4"
  }
}

Saibamen requested a review from a team as a code owner February 16, 2026 16:32
Saibamen changed the title Fix #8726 fix: skip invalid peerOptional edges in buildIdealTree problem detection Feb 16, 2026

ro0NL left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Copy link
Copy Markdown
Contributor Author

FYI: Right now, this PR fixes only npm ci after npm install.

package-lock.json file still has node_modules/node-fetch version 2.7.0 instead of 2.6.11 after first call to npm install.

I will try to fix this too.

…ptional

When a dependency is placed (OK) and creates an invalid peerOptional
edge with an already-processed node, re-queue that node for
re-resolution. This handles the case where the peerOptional holder
is processed before the dep exists in the tree (due to alphabetical
processing order), and the dep is later placed by another path with
a version that doesn't satisfy the peer spec.

Also adds npm install tests for issue npm#8726 covering both the
existing-lockfile and fresh-install (no lockfile) scenarios.

Refs: npm#8726
Co-authored-by: Cursor <cursoragent@cursor.com>

Copy link
Copy Markdown
Contributor Author

Now npm install produces a correct package-lock.json file.

Saibamen changed the title fix: skip invalid peerOptional edges in buildIdealTree problem detection fix(arborist): fix peerOptional dependency resolution in buildIdealTree Feb 16, 2026
Saibamen changed the title fix(arborist): fix peerOptional dependency resolution in buildIdealTree fix(arborist): fix peerOptional dependency resolution in buildIdealTree Feb 16, 2026
owlstronaut self-assigned this Feb 17, 2026

owlstronaut left a comment
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Could you unwrap the multi-line comments so each sentence/paragraph is on a single line? We avoid hard-wrapping prose in comments for accessibility as it doesn't play well with screen readers (we've been slowly converting the codebase to that anyway, it is still pretty prevalent)

Saibamen commented Feb 18, 2026
edited
Loading

Copy link
Copy Markdown
Contributor Author

Done in 95dc220 and 9c703f8

Copy link
Copy Markdown

@Saibamen looks like it needs 2 more lines of code coverage from tests as well

G-Rath commented Feb 18, 2026
edited
Loading

Copy link
Copy Markdown
Contributor

sounds like this should also resolve #6787, so @Saibamen might want to put that in the PR title body so that gets linked and closed when this is landed.

Thanks for working on this ❤️

Saibamen commented Feb 18, 2026
edited
Loading

Copy link
Copy Markdown
Contributor Author

put that in the PR title so that gets linked and closed when this is landed.

Linking doesn't work in PR title - you need to have it inside PR description (first comment).
And you can't use Fixed #x, #y for multiple issues... Every issue needs to be prefixed with keyword: https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

G-Rath commented Feb 18, 2026

Copy link
Copy Markdown
Contributor

yup I mistyped - I meant "body" rather than "title", and for you to do a "Fixes #xyz" for each issue 🙂

Saibamen and others added 3 commits February 18, 2026 19:36
…from the !edge.valid peerOptional check. This branch was unreachable because add: [...] changes peerOptional edges to regular dependency edges before they're added to #explicitRequests, so a peerOptional edge can never be both explicitly requested and still typed as peerOptional. The simplified condition edge.type !== 'peerOptional' || this.options.save !== false still correctly handles all reachable cases.
…=false

This test verifies that invalid peerOptional edges are not treated as problems when the save option is set to false, mimicking the behavior of `npm ci`. It ensures that the dependency tree is built correctly without raising issues for optional peer dependencies that do not meet the specified version requirements.

Copy link
Copy Markdown
Contributor Author

@owlstronaut Please run GH Actions again - I have 100% CC on my local git repo

ro0NL mentioned this pull request Feb 19, 2026

ro0NL commented Feb 19, 2026

Copy link
Copy Markdown

hope this bc break will be fixed in the next release

Saibamen requested a review from owlstronaut February 19, 2026 14:02

owlstronaut left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@Saibamen Thank you for working on this, it is an incredible contribution. I'm very pleased to accept this into the codebase ❤️

Saibamen commented Feb 19, 2026
edited
Loading

Copy link
Copy Markdown
Contributor Author

Also fixes #4859, #5854 and #8674 (see my comments there).

And probably 3 more open issues from #6787, but they don't have repro steps with package.json file (at least not in the first comment). I will ping issue authors when 11.10.1 #8987 will be released.

delagen commented Feb 20, 2026
edited
Loading

Copy link
Copy Markdown

In my case peerDependenciesMeta[optional] seems still ignored and all optional peer deps of dependent package still installed by default

Sorry, seems it's Gitlab Private Registry issue with metadata

https://gitlab.com/gitlab-org/gitlab/-/issues/463822

Copy link
Copy Markdown

Thanks a lot for all the effort on this fix, I see it was added to Node 25 which eventually will flow into Node 26. Is the plan to also include this fix in Node 24?

thanks in advance

Copy link
Copy Markdown
Contributor Author

@owlstronaut: Can you add this to NodeJS LTS 24?

Copy link
Copy Markdown
Contributor Author

@rvillane: In latest NodeJS 24.14.1, NPM was updated to version 11.11.0

https://github.com/nodejs/node/releases/tag/v24.14.1

guarzo added a commit to guarzo/authGD that referenced this pull request Aug 24, 2026
)

`npm ci` failed on a clean checkout with EUSAGE, naming a version that
appears nowhere in package-lock.json:

    npm error Missing: esbuild@0.28.2 from lock file
    ... (+26 @esbuild/* platform packages, all 0.28.2)

while `npm install --package-lock-only --dry-run` reported "up to date".

The lockfile is not at fault and is unchanged here. `vitest`'s nested
`vite@8.2.2` declares esbuild as an *optional* peerDependency
(`^0.27.0 || ^0.28.0`, `peerDependenciesMeta.esbuild.optional = true`),
and nothing in the lock satisfies that edge at that location — which is
correct, because an unsatisfied optional peer should be skipped. Older
npm instead resolves it against the registry (arborist's `loadPeerSet`),
picks up whatever is current, then rejects the lock as out of sync. That
is also the install-vs-ci contradiction: the two commands build the tree
by different paths and disagree about optional peers.

Fixed upstream by npm/cli#8981 (npm 11.10.1) and npm/cli#9083
(npm 11.11.1). Node 24.15.0 is the first 24.x bundling npm >= 11.11.1.

CI was never affected: `.nvmrc` is bare `24`, so setup-node floats to
v24.19.0 / npm 11.17.0. The failure only reached environments sitting on
an old-but-in-range Node 24, which `engines.node: ">=24"` permitted.
Regenerating the lockfile under npm 11.17.0 reproduces it byte for byte;
regenerating under an affected npm instead commits esbuild 0.28.2 and 26
platform packages that CI does not install.

`.nvmrc` deliberately stays bare `24` — the repo pins by major, and
scripts/check-node-version.sh compares majors only, so it still passes.

Also corrects two references the new floor invalidated: the Dockerfile
comment that quoted "node >=24" (now states the floor without repeating
a value that can drift) and the docs/ops.md local-dev requirement.
FaZe-devx added a commit to mond-day/frotec-lp that referenced this pull request Aug 26, 2026
O lock foi gerado no Windows com npm 11.6.2, que sofre de um bug conhecido
(npm/cli#8981, corrigido na 11.10.1): ao pular dependencias opcionais de
outra plataforma, ele mantem as referencias a elas no lock mas descarta as
entradas dos pacotes.

Na pratica faltavam @emnapi/core e @emnapi/runtime, exigidos por
@img/sharp-wasm32 e @napi-rs/wasm-runtime. No Windows nada quebra, porque
npm install tolera a lacuna; no runner Linux e no build da imagem Docker,
npm ci recusa o lock por estar fora de sincronia com o package.json.

Lock regerado com npm 12, que resolve o grafo completo. Continua no formato
lockfileVersion 3 e nenhuma versao de dependencia direta mudou.

Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Back | FazBrowse Home | New Git URL