From dfbbed9a0ff5c5e2431f57b78273a972d1c4a102 Mon Sep 17 00:00:00 2001 From: Liam Mitchell Date: Sun, 14 Sep 2025 20:12:09 +0200 Subject: [PATCH] fix: calculate omit in diff --- workspaces/arborist/lib/arborist/reify.js | 47 ++++--------------- workspaces/arborist/lib/diff.js | 32 +++++++++++-- workspaces/arborist/lib/node.js | 12 +++++ .../test/arborist/reify.js.test.cjs | 1 - workspaces/arborist/test/arborist/reify.js | 17 ++++++- 5 files changed, 63 insertions(+), 46 deletions(-) diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index 5da8e72bfa567..ee381e809216f 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -42,7 +42,6 @@ const { defaultLockfileVersion } = Shrinkwrap const _retireShallowNodes = Symbol.for('retireShallowNodes') const _loadBundlesAndUpdateTrees = Symbol.for('loadBundlesAndUpdateTrees') const _submitQuickAudit = Symbol('submitQuickAudit') -const _addOmitsToTrashList = Symbol('addOmitsToTrashList') const _unpackNewModules = Symbol.for('unpackNewModules') const _build = Symbol.for('build') @@ -85,6 +84,7 @@ module.exports = cls => class Reifier extends cls { #dryRun #nmValidated = new Set() #omit + #omitted #retiredPaths = {} #retiredUnchanged = {} #savePrefix @@ -109,6 +109,7 @@ module.exports = cls => class Reifier extends cls { } this.#omit = new Set(options.omit) + this.#omitted = new Set() // start tracker block this.addTracker('reify') @@ -141,6 +142,10 @@ module.exports = cls => class Reifier extends cls { this.idealTree = oldTree } await this[_saveIdealTree](options) + // clean omitted + for (const node of this.#omitted) { + node.parent = null + } // clean up any trash that is still in the tree for (const path of this[_trashList]) { const loc = relpath(this.idealTree.realpath, path) @@ -315,7 +320,6 @@ module.exports = cls => class Reifier extends cls { ]], [_rollbackCreateSparseTree, [ _createSparseTree, - _addOmitsToTrashList, _loadShrinkwrapsAndUpdateTrees, _loadBundlesAndUpdateTrees, _submitQuickAudit, @@ -470,6 +474,8 @@ module.exports = cls => class Reifier extends cls { // find all the nodes that need to change between the actual // and ideal trees. this.diff = Diff.calculate({ + omit: this.#omit, + omitted: this.#omitted, shrinkwrapInflated: this.#shrinkwrapInflated, filterNodes, actual: this.actualTree, @@ -554,37 +560,6 @@ module.exports = cls => class Reifier extends cls { }) } - // adding to the trash list will skip reifying, and delete them - // if they are currently in the tree and otherwise untouched. - [_addOmitsToTrashList] () { - if (!this.#omit.size) { - return - } - - const timeEnd = time.start('reify:trashOmits') - for (const node of this.idealTree.inventory.values()) { - const { top } = node - - // if the top is not the root or workspace then we do not want to omit it - if (!top.isProjectRoot && !top.isWorkspace) { - continue - } - - // if a diff filter has been created, then we do not omit the node if the - // top node is not in that set - if (this.diff?.filterSet?.size && !this.diff.filterSet.has(top)) { - continue - } - - // omit node if the dep type matches any omit flags that were set - if (node.shouldOmit(this.#omit)) { - this[_addNodeToTrashList](node) - } - } - - timeEnd() - } - [_createSparseTree] () { const timeEnd = time.start('reify:createSparse') // if we call this fn again, we look for the previous list @@ -683,7 +658,6 @@ module.exports = cls => class Reifier extends cls { // reload the diff and sparse tree because the ideal tree changed .then(() => this[_diffTrees]()) .then(() => this[_createSparseTree]()) - .then(() => this[_addOmitsToTrashList]()) .then(() => this[_loadShrinkwrapsAndUpdateTrees]()) .then(timeEnd) } @@ -691,15 +665,10 @@ module.exports = cls => class Reifier extends cls { // create a symlink for Links, extract for Nodes // return the node object, since we usually want that // handle optional dep failures here - // If node is in trash list, skip it // If reifying fails, and the node is optional, add it and its optionalSet // to the trash list // Always return the node. [_reifyNode] (node) { - if (this[_trashList].has(node.path)) { - return node - } - const timeEnd = time.start(`reifyNode:${node.location}`) this.addTracker('reify', node.name, node.location) diff --git a/workspaces/arborist/lib/diff.js b/workspaces/arborist/lib/diff.js index fb94407bb0166..9f2d5aed47d07 100644 --- a/workspaces/arborist/lib/diff.js +++ b/workspaces/arborist/lib/diff.js @@ -11,7 +11,9 @@ const { existsSync } = require('node:fs') const ssri = require('ssri') class Diff { - constructor ({ actual, ideal, filterSet, shrinkwrapInflated }) { + constructor ({ actual, ideal, filterSet, shrinkwrapInflated, omit, omitted }) { + this.omit = omit + this.omitted = omitted this.filterSet = filterSet this.shrinkwrapInflated = shrinkwrapInflated this.children = [] @@ -36,6 +38,8 @@ class Diff { ideal, filterNodes = [], shrinkwrapInflated = new Set(), + omit = new Set(), + omitted = new Set(), }) { // if there's a filterNode, then: // - get the path from the root to the filterNode. The root or @@ -94,18 +98,28 @@ class Diff { } return depth({ - tree: new Diff({ actual, ideal, filterSet, shrinkwrapInflated }), + tree: new Diff({ actual, ideal, filterSet, shrinkwrapInflated, omit, omitted }), getChildren, leave, }) } } -const getAction = ({ actual, ideal }) => { +const getAction = ({ actual, ideal, omit, omitted }) => { if (!ideal) { return 'REMOVE' } + if (ideal.shouldOmit?.(omit)) { + omitted.add(ideal) + + if (actual) { + return 'REMOVE' + } + + return null + } + // bundled meta-deps are copied over to the ideal tree when we visit it, // so they'll appear to be missing here. There's no need to handle them // in the diff, though, because they'll be replaced at reify time anyway @@ -184,6 +198,8 @@ const getChildren = diff => { removed, filterSet, shrinkwrapInflated, + omit, + omitted, } = diff // Note: we DON'T diff fsChildren themselves, because they are either @@ -214,6 +230,8 @@ const getChildren = diff => { removed, filterSet, shrinkwrapInflated, + omit, + omitted, }) } @@ -232,12 +250,14 @@ const diffNode = ({ removed, filterSet, shrinkwrapInflated, + omit, + omitted, }) => { if (filterSet.size && !(filterSet.has(ideal) || filterSet.has(actual))) { return } - const action = getAction({ actual, ideal }) + const action = getAction({ actual, ideal, omit, omitted }) // if it's a match, then get its children // otherwise, this is the child diff node @@ -245,7 +265,7 @@ const diffNode = ({ if (action === 'REMOVE') { removed.push(actual) } - children.push(new Diff({ actual, ideal, filterSet, shrinkwrapInflated })) + children.push(new Diff({ actual, ideal, filterSet, shrinkwrapInflated, omit, omitted })) } else { unchanged.push(ideal) // !*! Weird dirty hack warning !*! @@ -285,6 +305,8 @@ const diffNode = ({ removed, filterSet, shrinkwrapInflated, + omit, + omitted, })) } } diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index 91c61fa09b414..1f67708a41ced 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -490,6 +490,18 @@ class Node { } shouldOmit (omitSet) { + if (!omitSet.size) { + return false + } + + const { top } = this + + // if the top is not the root or workspace then we do not want to omit it + if (!top.isProjectRoot && !top.isWorkspace) { + return false + } + + // omit node if the dep type matches any omit flags that were set return ( this.peer && omitSet.has('peer') || this.dev && omitSet.has('dev') || diff --git a/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs b/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs index b94ccc76df7f5..bfc07067a60d1 100644 --- a/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs +++ b/workspaces/arborist/tap-snapshots/test/arborist/reify.js.test.cjs @@ -17362,7 +17362,6 @@ Array [ "reify:retireShallow", "reify:save", "reify:trash", - "reify:trashOmits", "reify:unpack", "reify:unretire", "reifyNode:node_modules/@isaacs/testing-peer-deps-b", diff --git a/workspaces/arborist/test/arborist/reify.js b/workspaces/arborist/test/arborist/reify.js index a2944ceff4e62..eb805d3245933 100644 --- a/workspaces/arborist/test/arborist/reify.js +++ b/workspaces/arborist/test/arborist/reify.js @@ -394,6 +394,21 @@ t.test('dev, optional, devOptional flags and omissions', t => { })) }) +t.test('omit reports no diff on second run', async t => { + const path = fixture(t, 'testing-dev-optional-flags') + createRegistry(t, true) + const arb = newArb({ path }) + await arb.reify({ omit: ['dev'] }) + t.equal(arb.actualTree.children.get('once'), undefined, 'no once in tree') + t.ok(arb.diff.children.length, 'first reify has changes') + await arb.reify({ omit: ['dev'] }) + t.equal(arb.actualTree.children.get('once'), undefined, 'no once in tree') + t.notOk(arb.diff.children.length, 'second reify has no changes') + await arb.reify({}) + t.ok(arb.actualTree.children.get('once'), 'once in tree') + t.ok(arb.diff.children.length, 'removing omit has changes') +}) + t.test('omits when both dev and optional flags are set', t => { const path = 'testing-dev-optional-flags-2' const omits = [['dev'], ['optional']] @@ -1329,7 +1344,7 @@ t.test('workspaces', async t => { await t.test('workspaces only', async t => { createRegistry(t, false) const { root, a, b } = await runCase(t, { workspaces: ['a'] }) - t.equal(root.exists(), false, 'root') + t.equal(root.exists(), true, 'root') t.equal(a.exists(), false, 'a') t.equal(b.exists(), true, 'b') })