From cfdbb4a78505a4107ab9d1d2b5c2ba9650299ba7 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Fri, 20 Jun 2025 11:37:48 -0700 Subject: [PATCH] fix(arborist): use omit when building ideal tree, centralize shouldOmit logic --- .../arborist/lib/arborist/build-ideal-tree.js | 6 +- workspaces/arborist/lib/arborist/reify.js | 19 +---- workspaces/arborist/lib/audit-report.js | 54 +++++++------- workspaces/arborist/lib/node.js | 9 +++ .../test/arborist/build-ideal-tree.js | 44 +++++++++++ .../strict-engine-dev/package.json | 8 ++ .../fixtures/engine-omit-test/package.json | 7 ++ .../strict-engine-optional/package.json | 8 ++ .../optional-engine-omit-test/package.json | 7 ++ .../strict-engine-peer/package.json | 8 ++ .../peer-engine-omit-test/package.json | 7 ++ workspaces/arborist/test/node.js | 74 +++++++++++++++++++ 12 files changed, 205 insertions(+), 46 deletions(-) create mode 100644 workspaces/arborist/test/fixtures/engine-omit-test/node_modules/strict-engine-dev/package.json create mode 100644 workspaces/arborist/test/fixtures/engine-omit-test/package.json create mode 100644 workspaces/arborist/test/fixtures/optional-engine-omit-test/node_modules/strict-engine-optional/package.json create mode 100644 workspaces/arborist/test/fixtures/optional-engine-omit-test/package.json create mode 100644 workspaces/arborist/test/fixtures/peer-engine-omit-test/node_modules/strict-engine-peer/package.json create mode 100644 workspaces/arborist/test/fixtures/peer-engine-omit-test/package.json diff --git a/workspaces/arborist/lib/arborist/build-ideal-tree.js b/workspaces/arborist/lib/arborist/build-ideal-tree.js index a7e01fcf14801..56b347c3affbb 100644 --- a/workspaces/arborist/lib/arborist/build-ideal-tree.js +++ b/workspaces/arborist/lib/arborist/build-ideal-tree.js @@ -192,9 +192,11 @@ module.exports = cls => class IdealTreeBuilder extends cls { } async #checkEngineAndPlatform () { - const { engineStrict, npmVersion, nodeVersion } = this.options + const { engineStrict, npmVersion, nodeVersion, omit = [] } = this.options + const omitSet = new Set(omit) + for (const node of this.idealTree.inventory.values()) { - if (!node.optional) { + if (!node.optional && !node.shouldOmit(omitSet)) { try { // if devEngines is present in the root node we ignore the engines check if (!(node.isRoot && node.package.devEngines)) { diff --git a/workspaces/arborist/lib/arborist/reify.js b/workspaces/arborist/lib/arborist/reify.js index 796be4fa507c4..7f3fa461b0667 100644 --- a/workspaces/arborist/lib/arborist/reify.js +++ b/workspaces/arborist/lib/arborist/reify.js @@ -84,9 +84,7 @@ module.exports = cls => class Reifier extends cls { #bundleUnpacked = new Set() // the nodes we unpack to read their bundles #dryRun #nmValidated = new Set() - #omitDev - #omitPeer - #omitOptional + #omit #retiredPaths = {} #retiredUnchanged = {} #savePrefix @@ -110,10 +108,7 @@ module.exports = cls => class Reifier extends cls { throw er } - const omit = new Set(options.omit || []) - this.#omitDev = omit.has('dev') - this.#omitOptional = omit.has('optional') - this.#omitPeer = omit.has('peer') + this.#omit = new Set(options.omit) // start tracker block this.addTracker('reify') @@ -562,12 +557,11 @@ 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.#omitDev && !this.#omitOptional && !this.#omitPeer) { + if (!this.#omit.size) { return } const timeEnd = time.start('reify:trashOmits') - for (const node of this.idealTree.inventory.values()) { const { top } = node @@ -583,12 +577,7 @@ module.exports = cls => class Reifier extends cls { } // omit node if the dep type matches any omit flags that were set - if ( - node.peer && this.#omitPeer || - node.dev && this.#omitDev || - node.optional && this.#omitOptional || - node.devOptional && this.#omitOptional && this.#omitDev - ) { + if (node.shouldOmit(this.#omit)) { this[_addNodeToTrashList](node) } } diff --git a/workspaces/arborist/lib/audit-report.js b/workspaces/arborist/lib/audit-report.js index dbd9be8bd3865..672de6ac68d39 100644 --- a/workspaces/arborist/lib/audit-report.js +++ b/workspaces/arborist/lib/audit-report.js @@ -148,7 +148,7 @@ class AuditReport extends Map { if (!seen.has(k)) { const p = [] for (const node of this.tree.inventory.query('packageName', name)) { - if (!shouldAudit(node, this[_omit], this.filterSet)) { + if (!this.shouldAudit(node)) { continue } @@ -282,7 +282,7 @@ class AuditReport extends Map { const timeEnd = time.start('auditReport:getReport') try { - const body = prepareBulkData(this.tree, this[_omit], this.filterSet) + const body = this.prepareBulkData() log.silly('audit', 'bulk request', body) // no sense asking if we don't have anything to audit, @@ -309,37 +309,33 @@ class AuditReport extends Map { timeEnd() } } -} -// return true if we should audit this one -const shouldAudit = (node, omit, filterSet) => - !node.version ? false - : node.isRoot ? false - : filterSet && filterSet.size !== 0 && !filterSet.has(node) ? false - : omit.size === 0 ? true - : !( // otherwise, just ensure we're not omitting this one - node.dev && omit.has('dev') || - node.optional && omit.has('optional') || - node.devOptional && omit.has('dev') && omit.has('optional') || - node.peer && omit.has('peer') - ) - -const prepareBulkData = (tree, omit, filterSet) => { - const payload = {} - for (const name of tree.inventory.query('packageName')) { - const set = new Set() - for (const node of tree.inventory.query('packageName', name)) { - if (!shouldAudit(node, omit, filterSet)) { - continue - } + // return true if we should audit this one + shouldAudit (node) { + return !node.version ? false + : node.isRoot ? false + : this.filterSet && this.filterSet.size !== 0 && !this.filterSet.has(node) ? false + : this[_omit].size === 0 ? true + : !node.shouldOmit(this[_omit]) + } - set.add(node.version) - } - if (set.size) { - payload[name] = [...set] + prepareBulkData () { + const payload = {} + for (const name of this.tree.inventory.query('packageName')) { + const set = new Set() + for (const node of this.tree.inventory.query('packageName', name)) { + if (!this.shouldAudit(node)) { + continue + } + + set.add(node.version) + } + if (set.size) { + payload[name] = [...set] + } } + return payload } - return payload } module.exports = AuditReport diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index d067fe393a3be..91c61fa09b414 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -489,6 +489,15 @@ class Node { return false } + shouldOmit (omitSet) { + return ( + this.peer && omitSet.has('peer') || + this.dev && omitSet.has('dev') || + this.optional && omitSet.has('optional') || + this.devOptional && omitSet.has('optional') && omitSet.has('dev') + ) + } + getBundler (path = []) { // made a cycle, definitely not bundled! if (path.includes(this)) { diff --git a/workspaces/arborist/test/arborist/build-ideal-tree.js b/workspaces/arborist/test/arborist/build-ideal-tree.js index 0bd1fbfafc1ee..6bae9e8809c1c 100644 --- a/workspaces/arborist/test/arborist/build-ideal-tree.js +++ b/workspaces/arborist/test/arborist/build-ideal-tree.js @@ -4091,3 +4091,47 @@ t.test('should take devEngines in account', async t => { const tree = await buildIdeal(path) t.matchSnapshot(String(tree.meta)) }) + +t.test('engine checking respects omit flags', async t => { + const testFixture = resolve(fixtures, 'engine-omit-test') + + t.test('fail on engine mismatch in devDependencies without omit=dev', async t => { + await t.rejects(buildIdeal(testFixture, { + nodeVersion: '12.18.4', + engineStrict: true, + }), + { code: 'EBADENGINE' }, + 'should fail with EBADENGINE when devDependencies have engine mismatch' + ) + }) + + t.test('skip engine check for devDependencies with omit=dev', async t => { + // This should NOT throw an EBADENGINE error + await buildIdeal(testFixture, { + nodeVersion: '12.18.4', + engineStrict: true, + omit: ['dev'], + }) + t.pass('should succeed when omitting dev dependencies with engine mismatches') + }) + + t.test('skip engine check for optionalDependencies with omit=optional', async t => { + const optionalFixture = resolve(fixtures, 'optional-engine-omit-test') + await buildIdeal(optionalFixture, { + nodeVersion: '12.18.4', + engineStrict: true, + omit: ['optional'], + }) + t.pass('should succeed when omitting optional dependencies with engine mismatches') + }) + + t.test('skip engine check for peerDependencies with omit=peer', async t => { + const peerFixture = resolve(fixtures, 'peer-engine-omit-test') + await buildIdeal(peerFixture, { + nodeVersion: '12.18.4', + engineStrict: true, + omit: ['peer'], + }) + t.pass('should succeed when omitting peer dependencies with engine mismatches') + }) +}) diff --git a/workspaces/arborist/test/fixtures/engine-omit-test/node_modules/strict-engine-dev/package.json b/workspaces/arborist/test/fixtures/engine-omit-test/node_modules/strict-engine-dev/package.json new file mode 100644 index 0000000000000..5e7f43804b7dd --- /dev/null +++ b/workspaces/arborist/test/fixtures/engine-omit-test/node_modules/strict-engine-dev/package.json @@ -0,0 +1,8 @@ +{ + "name": "strict-engine-dev", + "version": "1.0.0", + "description": "A devDependency with strict engine requirements", + "engines": { + "node": ">=18.0.0" + } +} diff --git a/workspaces/arborist/test/fixtures/engine-omit-test/package.json b/workspaces/arborist/test/fixtures/engine-omit-test/package.json new file mode 100644 index 0000000000000..d6f46b366c420 --- /dev/null +++ b/workspaces/arborist/test/fixtures/engine-omit-test/package.json @@ -0,0 +1,7 @@ +{ + "name": "engine-omit-test", + "version": "1.0.0", + "devDependencies": { + "strict-engine-dev": "1.0.0" + } +} diff --git a/workspaces/arborist/test/fixtures/optional-engine-omit-test/node_modules/strict-engine-optional/package.json b/workspaces/arborist/test/fixtures/optional-engine-omit-test/node_modules/strict-engine-optional/package.json new file mode 100644 index 0000000000000..3d8d97b4f35e8 --- /dev/null +++ b/workspaces/arborist/test/fixtures/optional-engine-omit-test/node_modules/strict-engine-optional/package.json @@ -0,0 +1,8 @@ +{ + "name": "strict-engine-optional", + "version": "1.0.0", + "description": "An optionalDependency with strict engine requirements", + "engines": { + "node": ">=18.0.0" + } +} diff --git a/workspaces/arborist/test/fixtures/optional-engine-omit-test/package.json b/workspaces/arborist/test/fixtures/optional-engine-omit-test/package.json new file mode 100644 index 0000000000000..f2ee2977c4b83 --- /dev/null +++ b/workspaces/arborist/test/fixtures/optional-engine-omit-test/package.json @@ -0,0 +1,7 @@ +{ + "name": "optional-engine-omit-test", + "version": "1.0.0", + "optionalDependencies": { + "strict-engine-optional": "1.0.0" + } +} diff --git a/workspaces/arborist/test/fixtures/peer-engine-omit-test/node_modules/strict-engine-peer/package.json b/workspaces/arborist/test/fixtures/peer-engine-omit-test/node_modules/strict-engine-peer/package.json new file mode 100644 index 0000000000000..97f99cdfcbde2 --- /dev/null +++ b/workspaces/arborist/test/fixtures/peer-engine-omit-test/node_modules/strict-engine-peer/package.json @@ -0,0 +1,8 @@ +{ + "name": "strict-engine-peer", + "version": "1.0.0", + "description": "A peerDependency with strict engine requirements", + "engines": { + "node": ">=18.0.0" + } +} diff --git a/workspaces/arborist/test/fixtures/peer-engine-omit-test/package.json b/workspaces/arborist/test/fixtures/peer-engine-omit-test/package.json new file mode 100644 index 0000000000000..6c1fe95fffa1c --- /dev/null +++ b/workspaces/arborist/test/fixtures/peer-engine-omit-test/package.json @@ -0,0 +1,7 @@ +{ + "name": "peer-engine-omit-test", + "version": "1.0.0", + "peerDependencies": { + "strict-engine-peer": "1.0.0" + } +} diff --git a/workspaces/arborist/test/node.js b/workspaces/arborist/test/node.js index 7eb6c4eacce01..c56899abf17b6 100644 --- a/workspaces/arborist/test/node.js +++ b/workspaces/arborist/test/node.js @@ -3322,3 +3322,77 @@ t.test('should find inconsistency between the edge\'s override set and the targe t.end() }) + +t.test('shouldOmit method', t => { + t.test('dev dependency with dev omit', t => { + const node = new Node({ + pkg: { name: 'test' }, + path: '/test', + dummy: true, + }) + node.dev = true + t.equal(node.shouldOmit(new Set(['dev'])), true, 'should omit dev dependency when dev is omitted') + t.equal(node.shouldOmit(new Set(['optional'])), false, 'should not omit dev dependency when only optional is omitted') + t.end() + }) + + t.test('optional dependency with optional omit', t => { + const node = new Node({ + pkg: { name: 'test' }, + path: '/test', + dummy: true, + }) + node.optional = true + t.equal(node.shouldOmit(new Set(['optional'])), true, 'should omit optional dependency when optional is omitted') + t.equal(node.shouldOmit(new Set(['dev'])), false, 'should not omit optional dependency when only dev is omitted') + t.end() + }) + + t.test('peer dependency with peer omit', t => { + const node = new Node({ + pkg: { name: 'test' }, + path: '/test', + dummy: true, + }) + node.peer = true + t.equal(node.shouldOmit(new Set(['peer'])), true, 'should omit peer dependency when peer is omitted') + t.equal(node.shouldOmit(new Set(['dev'])), false, 'should not omit peer dependency when only dev is omitted') + t.end() + }) + + t.test('devOptional dependency', t => { + const node = new Node({ + pkg: { name: 'test' }, + path: '/test', + dummy: true, + }) + node.devOptional = true + t.equal(node.shouldOmit(new Set(['dev', 'optional'])), true, 'should omit devOptional when both dev and optional are omitted') + t.equal(node.shouldOmit(new Set(['dev'])), false, 'should not omit devOptional when only dev is omitted') + t.equal(node.shouldOmit(new Set(['optional'])), false, 'should not omit devOptional when only optional is omitted') + t.end() + }) + + t.test('regular dependency', t => { + const node = new Node({ + pkg: { name: 'test' }, + path: '/test', + dummy: true, + }) + t.equal(node.shouldOmit(new Set(['dev', 'optional', 'peer'])), false, 'should never omit regular dependencies') + t.end() + }) + + t.test('empty omit set', t => { + const node = new Node({ + pkg: { name: 'test' }, + path: '/test', + dummy: true, + }) + node.dev = true + t.equal(node.shouldOmit(new Set()), false, 'should not omit anything when omit set is empty') + t.end() + }) + + t.end() +})