fix: dep flag calculation #8645

liamcmitchell · 2025-10-05T16:06:00Z

reverts pruning added in fix: prune optional peer dependencies that are no longer explicitly depended on #8431, which incorrectly prunes deps flagged as peer and optional - these flags don't mean that this node is an optional peer!
reverts much of fix: corrects peer dependency flag propagation #8579, which I think mistakenly changed peer dep calculation logic
rewrites calcDepFlags
adds logic to avoid unsetting extraneous when following optional peer edges (how fix: prune optional peer dependencies that are no longer explicitly depended on #8431, should have been fixed)
updates my prev fix to avoid looking for missing optional peer deps (if ((!edge.to && edge.type !== 'peerOptional') || !edge.valid) {)
refactors dep flag unsetting and resetting into Node methods
removes shake out Link target timing issue test, which was testing code removed a while back
avoids omitting flakyselflink fixture when writing snapshots

Fixes #8535, the last issue being that encoding was incorrectly pruned on subsequent installs. See the below dependency diagram produced from latest branch (since 11.6.1).

flowchart
encoding_0_1_13["`[email protected] (peer, optional)`"]
node_fetch_2_7_0["`[email protected] (optional)`"]
node_fetch_2_7_0-.->|peerOptional|encoding_0_1_13
_mapbox_node_pre_gyp_1_0_11["`@mapbox/[email protected] (optional)`"]
_mapbox_node_pre_gyp_1_0_11-->|prod|node_fetch_2_7_0
_abandonware_bluetooth_hci_socket_0_5_3_12["`@abandonware/[email protected] (optional)`"]
_abandonware_bluetooth_hci_socket_0_5_3_12-->|prod|_mapbox_node_pre_gyp_1_0_11
_abandonware_bluetooth_hci_socket_0_5_3_12-->|prod|node_gyp_10_3_1
_abandonware_noble_1_9_2_26["`@abandonware/[email protected] (prod)`"]
_abandonware_noble_1_9_2_26-.->|optional|_abandonware_bluetooth_hci_socket_0_5_3_12
minipass_fetch_3_0_5["`[email protected] (optional)`"]
minipass_fetch_3_0_5-.->|optional|encoding_0_1_13
make_fetch_happen_13_0_1["`[email protected] (optional)`"]
make_fetch_happen_13_0_1-->|prod|minipass_fetch_3_0_5
node_gyp_10_3_1["`[email protected] (optional)`"]
node_gyp_10_3_1-->|prod|make_fetch_happen_13_0_1

Code to generate

function graph (tree) {
  const safe = (id) => id.replace(/[^\w]/g, '_')
  let code = 'flowchart\n'
  const set = new Set()
  const collect = (node) => {
    set.add(node)
    for (const edge of node.edgesIn) {
      if (!set.has(edge.from)) {
        collect(edge.from)
      }
    }
  }
  for (const node of tree.inventory.values()) {
    if (node.name === 'encoding') {
      collect(node)
    }
  }
  set.delete(tree.root)
  for (const node of set) {
    const flags = node.extraneous && ['extraneous']
      || [
        node.peer && 'peer',
        ...(node.devOptional && !node.dev && !node.optional
          ? ['devOptional']
          : [
            node.dev && 'dev',
            node.optional && 'optional',
          ]),
      ].filter(Boolean).join(', ')
      || 'prod'
    code += `${safe(node.pkgid)}["\`${node.pkgid} (${flags})\`"]\n`
    for (const edge of node.edgesOut.values()) {
      if (!set.has(edge.to)) continue
      code += `${safe(node.pkgid)}-${edge.optional ? '.' : ''}->|${[edge.type, edge.error].filter(Boolean).join(' ')}|${safe(edge.to?.pkgid || `${edge.name}@${edge.spec}`)}\n`
    }
  }
  const mermaid = JSON.stringify({ theme: 'neutral' })
  require('child_process').execSync(`open https://mermaid.live/edit#pako:${require('zlib').deflateSync(JSON.stringify({ code, mermaid })).toString('base64')}`)
}

Note that encoding should not have the peer flag. This incorrect flag plus the incorrect pruning of optional && peer when building ideal tree from shrinkwrap causes this node to be removed from the tree. encodings de-deduped deps (not shown) were NOT removed and when checkEngineAndPlatform marks @abandonware/bluetooth-hci-socket as inert, it cannot follow the removed node to also mark these deps as inert. No longer inert, the diff algorithm now sees them as missing and they are installed.

liamcmitchell · 2025-10-05T17:07:20Z

A few more words on dep flags...

Their intention is documented here: https://github.com/npm/cli/blob/latest/workspaces/arborist/README.md#package-dependency-flags

relevant when pruning nodes from the tree

Pruning means both the prune command which removes extraneous nodes and the production/omit/include flags which filter a subset of deps.

The optional flag is also used to decide if a dep installation failure can be ignored but because of the intended use in pruning, the optional flag might not always allow failure as expected.

Consider the following

tree (dev A, optional B)
+-- A (prod C) dev: true
+-- B (optional C) optional: true
+-- C dev: false, optional: false, devOptional: true

C is not dev or optional because it won't be pruned as either. It is devOptional because it will be pruned if both dev and optional are omitted together.

If C installation fails in npm install --omit=dev... the failure will be fatal because C is not optional - even though the only prod dependent wasn't installed.

Maybe another edge case that can be fixed by recalculating dep flags again after diffing? Or is it before diffing? After we check if a package is omitted but before we check if it is installable?

I think there are a lot of subtle bugs caused by this fragile pre-calculation of dep flags and trying to keep it in sync.

Why are dep flags pre-calculated at all? We only need them when pruning or handling install failures. Could this be done lazily in diff/reify?

Why are dep flags persisted to lockfiles and not just re-calculated? Performance? Curiosity? Preserving inconsistent dep flag calculations across different npm versions? 🤔

wraithgar · 2025-10-06T16:18:26Z

Why are dep flags persisted to lockfiles and not just re-calculated?

I believe the reason is so that things like --omit=dev work from the shrinkwrap and not from re-calculating the tree.

ref:
npm/npm@1eabcd1
and
npm/npm#10073

Why are dep flags pre-calculated at all? We only need them when pruning or handling install failures. Could this be done lazily in diff/reify?

Do we have enough info from a shrinkwrap install to still calculate all of this without the flags IN the shrinkwrap? Does this question even make sense anymore (i.e. are we currently still pulling in the same info anyways that we would if the flags weren't in the shrinkwrap?)

These are all good questions you have. I think this is a case of things making sense at the time, and the software having iterated enough over time that we can rethink some decisions.

Removing these flags from the lockfile is a pretty significant breaking change, however. Making changes that aren't breaking would be the challenge here. Can we still persist them but have the new code ignore them?

liamcmitchell · 2025-10-12T07:58:04Z

Do we have enough info from a shrinkwrap install to still calculate all of this without the flags IN the shrinkwrap?

Yes, flags are always calculated from a tree. If shrinkwrap provides a tree, deps can be calculated.

There are already many checks in shrinkwrap load that may set flagsSuspect and trigger re-calc of dep flags.

Can we still persist them but have the new code ignore them?

Yes, very possible. Would be interesting to see if there is much of a performance difference too.

I definitely won't be doing this in this PR and doubt I'll have any time for this in the near future.

wraithgar · 2025-10-12T16:01:01Z

I definitely won't be doing this in this PR and doubt I'll have any time for this in the near future.

Yep understood, that's a much larger task than these cleanup PRs.

wraithgar · 2025-10-13T04:55:29Z

workspaces/arborist/lib/arborist/load-virtual.js

-        node.peer = true
+        node.resetDepFlags()
      }
      calcDepFlags(this.virtualTree, !this.#rootOptionProvided)


I don't think it needs to be part of this PR but we definitely have some duplicate/inefficient logic going on w/ this #rootOptionProvided flag and the resetRoot parameter. I will make a note to follow up after this PR clean some things up.

For instance in this block flagsSuspect is only true if rootOptionProvided is not true.

fix: dep flag calculation

386590b

liamcmitchell requested a review from a team as a code owner October 5, 2025 16:06

wraithgar self-assigned this Oct 6, 2025

wraithgar mentioned this pull request Oct 6, 2025

[EPIC]: Fix incorrect ERESOLVE errors from peer dependencies #8647

Closed

This was referenced Oct 12, 2025

[BUG] Directly requested packages incorrectly marked as peer and not installed with --omit-peer #8666

Open

[BUG] v11.6.1 fails to install optionalDependencies of inner package (even if build works) #8628

Open

wraithgar reviewed Oct 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: dep flag calculation #8645

fix: dep flag calculation #8645

Uh oh!

liamcmitchell commented Oct 5, 2025 •

edited

Loading

Uh oh!

liamcmitchell commented Oct 5, 2025

Uh oh!

wraithgar commented Oct 6, 2025

Uh oh!

liamcmitchell commented Oct 12, 2025

Uh oh!

wraithgar commented Oct 12, 2025

Uh oh!

wraithgar Oct 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fix: dep flag calculation #8645

Are you sure you want to change the base?

fix: dep flag calculation #8645

Uh oh!

Conversation

liamcmitchell commented Oct 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liamcmitchell commented Oct 5, 2025

Uh oh!

wraithgar commented Oct 6, 2025

Uh oh!

liamcmitchell commented Oct 12, 2025

Uh oh!

wraithgar commented Oct 12, 2025

Uh oh!

wraithgar Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

liamcmitchell commented Oct 5, 2025 •

edited

Loading