From 6f7abda03e81ee01f8c39cd1aac65552ccfd25af Mon Sep 17 00:00:00 2001 From: AmirSa12 Date: Sat, 1 Jun 2024 09:36:34 +0330 Subject: [PATCH 1/7] fix node.target when it is null --- workspaces/arborist/lib/calc-dep-flags.js | 123 +++++++++++++--------- 1 file changed, 71 insertions(+), 52 deletions(-) diff --git a/workspaces/arborist/lib/calc-dep-flags.js b/workspaces/arborist/lib/calc-dep-flags.js index 45ed9562479af..2c76a2df72783 100644 --- a/workspaces/arborist/lib/calc-dep-flags.js +++ b/workspaces/arborist/lib/calc-dep-flags.js @@ -1,119 +1,138 @@ -const { depth } = require('treeverse') +const { depth } = require("treeverse"); const calcDepFlags = (tree, resetRoot = true) => { if (resetRoot) { - tree.dev = false - tree.optional = false - tree.devOptional = false - tree.peer = false + tree.dev = false; + tree.optional = false; + tree.devOptional = false; + tree.peer = false; } const ret = depth({ tree, - visit: node => calcDepFlagsStep(node), - filter: node => node, + visit: (node) => calcDepFlagsStep(node), + filter: (node) => node, getChildren: (node, tree) => - [...tree.edgesOut.values()].map(edge => edge.to), - }) - return ret -} + [...tree.edgesOut.values()].map((edge) => edge.to), + }); + return ret; +}; const calcDepFlagsStep = (node) => { // This rewalk is necessary to handle cases where devDep and optional // or normal dependency graphs overlap deep in the dep graph. // Since we're only walking through deps that are not already flagged // as non-dev/non-optional, it's typically a very shallow traversal - node.extraneous = false - resetParents(node, 'extraneous') - resetParents(node, 'dev') - resetParents(node, 'peer') - resetParents(node, 'devOptional') - resetParents(node, 'optional') + node.extraneous = false; + resetParents(node, "extraneous"); + resetParents(node, "dev"); + resetParents(node, "peer"); + resetParents(node, "devOptional"); + resetParents(node, "optional"); // for links, map their hierarchy appropriately if (node.isLink) { - node.target.dev = node.dev - node.target.optional = node.optional - node.target.devOptional = node.devOptional - node.target.peer = node.peer - return calcDepFlagsStep(node.target) + // node.target can be null, we check to ensure it's not null before proceeding + if (node.target == null) { + return node; + } + node.target.dev = node.dev; + node.target.optional = node.optional; + node.target.devOptional = node.devOptional; + node.target.peer = node.peer; + return calcDepFlagsStep(node.target); } node.edgesOut.forEach(({ peer, optional, dev, to }) => { // if the dep is missing, then its flags are already maximally unset if (!to) { - return + return; } // everything with any kind of edge into it is not extraneous - to.extraneous = false + to.extraneous = false; // devOptional is the *overlap* of the dev and optional tree. // however, for convenience and to save an extra rewalk, we leave // it set when we are in *either* tree, and then omit it from the // package-lock if either dev or optional are set. - const unsetDevOpt = !node.devOptional && !node.dev && !node.optional && !dev && !optional + const unsetDevOpt = + !node.devOptional && !node.dev && !node.optional && !dev && !optional; // if we are not in the devOpt tree, then we're also not in // either the dev or opt trees - const unsetDev = unsetDevOpt || !node.dev && !dev - const unsetOpt = unsetDevOpt || !node.optional && !optional - const unsetPeer = !node.peer && !peer + const unsetDev = unsetDevOpt || (!node.dev && !dev); + const unsetOpt = unsetDevOpt || (!node.optional && !optional); + const unsetPeer = !node.peer && !peer; if (unsetPeer) { - unsetFlag(to, 'peer') + unsetFlag(to, "peer"); } if (unsetDevOpt) { - unsetFlag(to, 'devOptional') + unsetFlag(to, "devOptional"); } if (unsetDev) { - unsetFlag(to, 'dev') + unsetFlag(to, "dev"); } if (unsetOpt) { - unsetFlag(to, 'optional') + unsetFlag(to, "optional"); } - }) + }); - return node -} + return node; +}; const resetParents = (node, flag) => { if (node[flag]) { - return + return; } for (let p = node; p && (p === node || p[flag]); p = p.resolveParent) { - p[flag] = false + p[flag] = false; } -} +}; // typically a short walk, since it only traverses deps that have the flag set. const unsetFlag = (node, flag) => { if (node[flag]) { - node[flag] = false + node[flag] = false; depth({ tree: node, - visit: node => { - node.extraneous = node[flag] = false + visit: (node) => { + node.extraneous = node[flag] = false; if (node.isLink) { - node.target.extraneous = node.target[flag] = false + // node.target can be null, we check to ensure it's not null before proceeding + if (node.target == null) { + return []; + } else { + node.target.extraneous = node.target[flag] = false; + } } }, - getChildren: node => { - const children = [] - for (const edge of node.target.edgesOut.values()) { - if (edge.to && edge.to[flag] && - (flag !== 'peer' && edge.type === 'peer' || edge.type === 'prod') + getChildren: (node) => { + if (node.isLink && node.target == null) { + return []; + } + const children = []; + const targetNode = node.isLink ? node.target : node; + if (targetNode == null) { + return []; + } + for (const edge of targetNode.edgesOut.values()) { + if ( + edge.to && + edge.to[flag] && + ((flag !== "peer" && edge.type === "peer") || edge.type === "prod") ) { - children.push(edge.to) + children.push(edge.to); } } - return children + return children; }, - }) + }); } -} +}; -module.exports = calcDepFlags +module.exports = calcDepFlags; From 73d1bad84a0d4e7b4a1b581f99208e38d91b8f1d Mon Sep 17 00:00:00 2001 From: AmirSa12 Date: Sat, 1 Jun 2024 10:36:12 +0330 Subject: [PATCH 2/7] remove all the unrelated style changes caused by auto formatting --- workspaces/arborist/lib/calc-dep-flags.js | 111 +++++++++++----------- 1 file changed, 55 insertions(+), 56 deletions(-) diff --git a/workspaces/arborist/lib/calc-dep-flags.js b/workspaces/arborist/lib/calc-dep-flags.js index 2c76a2df72783..da51cafaa36bf 100644 --- a/workspaces/arborist/lib/calc-dep-flags.js +++ b/workspaces/arborist/lib/calc-dep-flags.js @@ -1,138 +1,137 @@ -const { depth } = require("treeverse"); +const { depth } = require('treeverse') const calcDepFlags = (tree, resetRoot = true) => { if (resetRoot) { - tree.dev = false; - tree.optional = false; - tree.devOptional = false; - tree.peer = false; + tree.dev = false + tree.optional = false + tree.devOptional = false + tree.peer = false } const ret = depth({ tree, - visit: (node) => calcDepFlagsStep(node), - filter: (node) => node, + visit: node => calcDepFlagsStep(node), + filter: node => node, getChildren: (node, tree) => - [...tree.edgesOut.values()].map((edge) => edge.to), - }); - return ret; -}; + [...tree.edgesOut.values()].map(edge => edge.to), + }) + return ret +} const calcDepFlagsStep = (node) => { // This rewalk is necessary to handle cases where devDep and optional // or normal dependency graphs overlap deep in the dep graph. // Since we're only walking through deps that are not already flagged // as non-dev/non-optional, it's typically a very shallow traversal - node.extraneous = false; - resetParents(node, "extraneous"); - resetParents(node, "dev"); - resetParents(node, "peer"); - resetParents(node, "devOptional"); - resetParents(node, "optional"); + node.extraneous = false + resetParents(node, 'extraneous') + resetParents(node, 'dev') + resetParents(node, 'peer') + resetParents(node, 'devOptional') + resetParents(node, 'optional') // for links, map their hierarchy appropriately if (node.isLink) { // node.target can be null, we check to ensure it's not null before proceeding if (node.target == null) { - return node; + return node } - node.target.dev = node.dev; - node.target.optional = node.optional; - node.target.devOptional = node.devOptional; - node.target.peer = node.peer; - return calcDepFlagsStep(node.target); + node.target.dev = node.dev + node.target.optional = node.optional + node.target.devOptional = node.devOptional + node.target.peer = node.peer + return calcDepFlagsStep(node.target) } node.edgesOut.forEach(({ peer, optional, dev, to }) => { // if the dep is missing, then its flags are already maximally unset if (!to) { - return; + return } // everything with any kind of edge into it is not extraneous - to.extraneous = false; + to.extraneous = false // devOptional is the *overlap* of the dev and optional tree. // however, for convenience and to save an extra rewalk, we leave // it set when we are in *either* tree, and then omit it from the // package-lock if either dev or optional are set. - const unsetDevOpt = - !node.devOptional && !node.dev && !node.optional && !dev && !optional; + const unsetDevOpt = !node.devOptional && !node.dev && !node.optional && !dev && !optional // if we are not in the devOpt tree, then we're also not in // either the dev or opt trees - const unsetDev = unsetDevOpt || (!node.dev && !dev); - const unsetOpt = unsetDevOpt || (!node.optional && !optional); - const unsetPeer = !node.peer && !peer; + const unsetDev = unsetDevOpt || !node.dev && !dev + const unsetOpt = unsetDevOpt || !node.optional && !optional + const unsetPeer = !node.peer && !peer if (unsetPeer) { - unsetFlag(to, "peer"); + unsetFlag(to, 'peer') } if (unsetDevOpt) { - unsetFlag(to, "devOptional"); + unsetFlag(to, 'devOptional') } if (unsetDev) { - unsetFlag(to, "dev"); + unsetFlag(to, 'dev') } if (unsetOpt) { - unsetFlag(to, "optional"); + unsetFlag(to, 'optional') } - }); + }) - return node; -}; + return node +} const resetParents = (node, flag) => { if (node[flag]) { - return; + return } for (let p = node; p && (p === node || p[flag]); p = p.resolveParent) { - p[flag] = false; + p[flag] = false } -}; +} // typically a short walk, since it only traverses deps that have the flag set. const unsetFlag = (node, flag) => { if (node[flag]) { - node[flag] = false; + node[flag] = false depth({ tree: node, - visit: (node) => { - node.extraneous = node[flag] = false; + visit: node => { + node.extraneous = node[flag] = false if (node.isLink) { // node.target can be null, we check to ensure it's not null before proceeding if (node.target == null) { - return []; + return [] } else { - node.target.extraneous = node.target[flag] = false; + node.target.extraneous = node.target[flag] = false } } }, - getChildren: (node) => { + getChildren: node => { if (node.isLink && node.target == null) { - return []; + return [] } - const children = []; - const targetNode = node.isLink ? node.target : node; + const children = [] + const targetNode = node.isLink ? node.target : node if (targetNode == null) { - return []; + return [] } for (const edge of targetNode.edgesOut.values()) { if ( edge.to && edge.to[flag] && - ((flag !== "peer" && edge.type === "peer") || edge.type === "prod") + ((flag !== 'peer' && edge.type === 'peer') || edge.type === 'prod') ) { - children.push(edge.to); + children.push(edge.to) } } - return children; + return children }, - }); + }) } -}; +} -module.exports = calcDepFlags; +module.exports = calcDepFlags From 03d13b0057e5d26b171b23515475cd0a184bf342 Mon Sep 17 00:00:00 2001 From: AmirSa12 Date: Sun, 2 Jun 2024 12:39:49 +0330 Subject: [PATCH 3/7] add tests --- workspaces/arborist/test/calc-dep-flags.js | 39 ++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/workspaces/arborist/test/calc-dep-flags.js b/workspaces/arborist/test/calc-dep-flags.js index bba64fc5dd0dc..75f8515912bd8 100644 --- a/workspaces/arborist/test/calc-dep-flags.js +++ b/workspaces/arborist/test/calc-dep-flags.js @@ -264,3 +264,42 @@ t.test('set parents to not extraneous when visiting', t => { t.equal(bazLink.devOptional, false, 'bazlink not devOptional') t.end() }) + +t.test('check null target in link', async t => { + const root = new Link({ + path: '/some/path', + realpath: '/some/path', + pkg: { + dependencies: { foo: '' }, + }, + }); + + if (root.target == null) { + t.equal(root.target, null, 'root.target should be null'); + t.equal(root, root, 'should return the node itself if target is null'); + } else { + root.target.peer = true; + } + + const nonNullTarget = new Node({ + name: "notNull", + path: '/some/path/node', + }) + + const rootWithNonNullTarget = new Link({ + name: "notNull", + path: '/some/path/non/null', + pkg: { + dependencies: { foo: '' }, + }, + target: nonNullTarget + }); + if (rootWithNonNullTarget.target == null) { + t.equal(rootWithNonNullTarget.target, null, 'rootWithNonNullTarget.target should be null'); + t.equal(rootWithNonNullTarget, rootWithNonNullTarget, 'should return the node itself if target is null'); + } else { + rootWithNonNullTarget.target.peer = true; + t.equal(rootWithNonNullTarget.target.peer, true, 'rootWithNonNullTarget.target.peer should be true'); + } + t.end(); +}); \ No newline at end of file From de861de81100d82c07dc83778d181c8883406bd7 Mon Sep 17 00:00:00 2001 From: AmirSa12 Date: Tue, 4 Jun 2024 19:48:45 +0330 Subject: [PATCH 4/7] failing test --- workspaces/arborist/lib/calc-dep-flags.js | 26 +++---------------- workspaces/arborist/test/calc-dep-flags.js | 30 ++-------------------- 2 files changed, 6 insertions(+), 50 deletions(-) diff --git a/workspaces/arborist/lib/calc-dep-flags.js b/workspaces/arborist/lib/calc-dep-flags.js index da51cafaa36bf..45ed9562479af 100644 --- a/workspaces/arborist/lib/calc-dep-flags.js +++ b/workspaces/arborist/lib/calc-dep-flags.js @@ -31,10 +31,6 @@ const calcDepFlagsStep = (node) => { // for links, map their hierarchy appropriately if (node.isLink) { - // node.target can be null, we check to ensure it's not null before proceeding - if (node.target == null) { - return node - } node.target.dev = node.dev node.target.optional = node.optional node.target.devOptional = node.devOptional @@ -102,28 +98,14 @@ const unsetFlag = (node, flag) => { visit: node => { node.extraneous = node[flag] = false if (node.isLink) { - // node.target can be null, we check to ensure it's not null before proceeding - if (node.target == null) { - return [] - } else { - node.target.extraneous = node.target[flag] = false - } + node.target.extraneous = node.target[flag] = false } }, getChildren: node => { - if (node.isLink && node.target == null) { - return [] - } const children = [] - const targetNode = node.isLink ? node.target : node - if (targetNode == null) { - return [] - } - for (const edge of targetNode.edgesOut.values()) { - if ( - edge.to && - edge.to[flag] && - ((flag !== 'peer' && edge.type === 'peer') || edge.type === 'prod') + for (const edge of node.target.edgesOut.values()) { + if (edge.to && edge.to[flag] && + (flag !== 'peer' && edge.type === 'peer' || edge.type === 'prod') ) { children.push(edge.to) } diff --git a/workspaces/arborist/test/calc-dep-flags.js b/workspaces/arborist/test/calc-dep-flags.js index 75f8515912bd8..966e7cce8d87d 100644 --- a/workspaces/arborist/test/calc-dep-flags.js +++ b/workspaces/arborist/test/calc-dep-flags.js @@ -266,6 +266,7 @@ t.test('set parents to not extraneous when visiting', t => { }) t.test('check null target in link', async t => { + const root = new Link({ path: '/some/path', realpath: '/some/path', @@ -273,33 +274,6 @@ t.test('check null target in link', async t => { dependencies: { foo: '' }, }, }); - - if (root.target == null) { - t.equal(root.target, null, 'root.target should be null'); - t.equal(root, root, 'should return the node itself if target is null'); - } else { - root.target.peer = true; - } - - const nonNullTarget = new Node({ - name: "notNull", - path: '/some/path/node', - }) - - const rootWithNonNullTarget = new Link({ - name: "notNull", - path: '/some/path/non/null', - pkg: { - dependencies: { foo: '' }, - }, - target: nonNullTarget - }); - if (rootWithNonNullTarget.target == null) { - t.equal(rootWithNonNullTarget.target, null, 'rootWithNonNullTarget.target should be null'); - t.equal(rootWithNonNullTarget, rootWithNonNullTarget, 'should return the node itself if target is null'); - } else { - rootWithNonNullTarget.target.peer = true; - t.equal(rootWithNonNullTarget.target.peer, true, 'rootWithNonNullTarget.target.peer should be true'); - } + t.doesNotThrow(()=> calcDepFlags(root)) t.end(); }); \ No newline at end of file From 866a83ed39801472c96c2f13aae89f7aa4b9ad57 Mon Sep 17 00:00:00 2001 From: AmirSa12 Date: Tue, 4 Jun 2024 23:47:57 +0330 Subject: [PATCH 5/7] success test --- workspaces/arborist/lib/calc-dep-flags.js | 13 ++++++++++--- workspaces/arborist/test/calc-dep-flags.js | 6 +++--- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/workspaces/arborist/lib/calc-dep-flags.js b/workspaces/arborist/lib/calc-dep-flags.js index 45ed9562479af..50cebeab64191 100644 --- a/workspaces/arborist/lib/calc-dep-flags.js +++ b/workspaces/arborist/lib/calc-dep-flags.js @@ -31,6 +31,10 @@ const calcDepFlagsStep = (node) => { // for links, map their hierarchy appropriately if (node.isLink) { + // node.target can be null, we check to ensure it's not null before proceeding + if (node.target == null) { + return node + } node.target.dev = node.dev node.target.optional = node.optional node.target.devOptional = node.devOptional @@ -103,9 +107,12 @@ const unsetFlag = (node, flag) => { }, getChildren: node => { const children = [] - for (const edge of node.target.edgesOut.values()) { - if (edge.to && edge.to[flag] && - (flag !== 'peer' && edge.type === 'peer' || edge.type === 'prod') + const targetNode = node.isLink ? node.target : node + for (const edge of targetNode.edgesOut.values()) { + if ( + edge.to && + edge.to[flag] && + ((flag !== 'peer' && edge.type === 'peer') || edge.type === 'prod') ) { children.push(edge.to) } diff --git a/workspaces/arborist/test/calc-dep-flags.js b/workspaces/arborist/test/calc-dep-flags.js index 966e7cce8d87d..eb80e40545a13 100644 --- a/workspaces/arborist/test/calc-dep-flags.js +++ b/workspaces/arborist/test/calc-dep-flags.js @@ -265,8 +265,7 @@ t.test('set parents to not extraneous when visiting', t => { t.end() }) -t.test('check null target in link', async t => { - +t.test('check null target in link', async t => { const root = new Link({ path: '/some/path', realpath: '/some/path', @@ -274,6 +273,7 @@ t.test('check null target in link', async t => { dependencies: { foo: '' }, }, }); - t.doesNotThrow(()=> calcDepFlags(root)) + t.doesNotThrow(() => calcDepFlags(root)) + t.doesNotThrow(() => calcDepFlags(root, false)) t.end(); }); \ No newline at end of file From a128db4ba60a5c4088fdafa734071a491e224962 Mon Sep 17 00:00:00 2001 From: AmirSa12 Date: Wed, 5 Jun 2024 00:13:23 +0330 Subject: [PATCH 6/7] fix getChildren --- workspaces/arborist/lib/calc-dep-flags.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/workspaces/arborist/lib/calc-dep-flags.js b/workspaces/arborist/lib/calc-dep-flags.js index 50cebeab64191..75749993ebd19 100644 --- a/workspaces/arborist/lib/calc-dep-flags.js +++ b/workspaces/arborist/lib/calc-dep-flags.js @@ -101,13 +101,13 @@ const unsetFlag = (node, flag) => { tree: node, visit: node => { node.extraneous = node[flag] = false - if (node.isLink) { + if (node.isLink && node.target) { node.target.extraneous = node.target[flag] = false } }, getChildren: node => { const children = [] - const targetNode = node.isLink ? node.target : node + const targetNode = node.isLink && node.target ? node.target : node; for (const edge of targetNode.edgesOut.values()) { if ( edge.to && From 4b31759ae6f938a7bf956d8b730eaf84c99ff0fa Mon Sep 17 00:00:00 2001 From: AmirSa12 Date: Wed, 5 Jun 2024 10:36:19 +0330 Subject: [PATCH 7/7] lint --- workspaces/arborist/lib/calc-dep-flags.js | 2 +- workspaces/arborist/test/calc-dep-flags.js | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/workspaces/arborist/lib/calc-dep-flags.js b/workspaces/arborist/lib/calc-dep-flags.js index 75749993ebd19..bcd30d0f493c7 100644 --- a/workspaces/arborist/lib/calc-dep-flags.js +++ b/workspaces/arborist/lib/calc-dep-flags.js @@ -107,7 +107,7 @@ const unsetFlag = (node, flag) => { }, getChildren: node => { const children = [] - const targetNode = node.isLink && node.target ? node.target : node; + const targetNode = node.isLink && node.target ? node.target : node for (const edge of targetNode.edgesOut.values()) { if ( edge.to && diff --git a/workspaces/arborist/test/calc-dep-flags.js b/workspaces/arborist/test/calc-dep-flags.js index eb80e40545a13..ff7f320ded29d 100644 --- a/workspaces/arborist/test/calc-dep-flags.js +++ b/workspaces/arborist/test/calc-dep-flags.js @@ -265,15 +265,15 @@ t.test('set parents to not extraneous when visiting', t => { t.end() }) -t.test('check null target in link', async t => { +t.test('check null target in link', async t => { const root = new Link({ path: '/some/path', realpath: '/some/path', pkg: { dependencies: { foo: '' }, }, - }); + }) t.doesNotThrow(() => calcDepFlags(root)) t.doesNotThrow(() => calcDepFlags(root, false)) - t.end(); -}); \ No newline at end of file + t.end() +})