Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wip: treat links with installLinks set as Links not Nodes #5820

Draft
wants to merge 2 commits into
base: latest
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions smoke-tests/test/install-links.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
const t = require('tap')
const { join } = require('path')
const setup = require('./fixtures/setup.js')

t.test('workspaces + file deps + non-deduping abbrev', async t => {
const abbrevs = ['1.0.3', '1.0.4', '1.0.5', '1.0.6']

const { npm, registry, paths } = await setup(t, {
debug: true,
testdir: {
a: {
'package.json': { name: 'a', dependencies: { abbrev: abbrevs[0], b: 'file:./b' } },
},
b: {
'package.json': { name: 'b', dependencies: { abbrev: abbrevs[1] } },
},
packages: abbrevs.reduce((acc, v) => {
acc[`abbrev-${v}`] = { 'package.json': { name: 'abbrev', version: v } }
return acc
}, {}),
},
})

await npm('init', '-y')
await npm('init', '-y', '--workspace=workspaces/ws-a')
await npm('init', '-y', '--workspace=workspaces/ws-b')

// set root deps
await npm('pkg', 'set', `dependencies.ws-a=1.0.0`)
await npm('pkg', 'set', `dependencies.a=file:../a`)

// set workspace a deps
await npm('pkg', 'set', `dependencies.abbrev=${abbrevs[2]}`, '--workspace=ws-a')
await npm('pkg', 'set', `dependencies.ws-b=1.0.0`, '--workspace=ws-a')

// set workspace b deps
await npm('pkg', 'set', `dependencies.abbrev=${abbrevs[3]}`, '--workspace=ws-b')

await registry.package({
manifest: registry.manifest({ name: 'abbrev', versions: abbrevs }),
tarballs: abbrevs.reduce((acc, v) => {
acc[v] = join(paths.root, 'packages', `abbrev-${v}`)
return acc
}, {}),
times: 2,

})

// this should not fail and all 4 abbrevs should get installed
// with no mocks remaining
await npm('install', '--install-links=true')
})
41 changes: 34 additions & 7 deletions workspaces/arborist/lib/arborist/build-ideal-tree.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const Shrinkwrap = require('../shrinkwrap.js')
const { defaultLockfileVersion } = Shrinkwrap
const Node = require('../node.js')
const Link = require('../link.js')
const Edge = require('../edge.js')
const addRmPkgDeps = require('../add-rm-pkg-deps.js')
const optionalSet = require('../optional-set.js')
const { checkEngine, checkPlatform } = require('npm-install-checks')
Expand Down Expand Up @@ -1045,14 +1046,18 @@ This is a one-time fix-up, please be patient...
// loads a node from an edge, and then loads its peer deps (and their
// peer deps, on down the line) into a virtual root parent.
async [_nodeFromEdge] (edge, parent_, secondEdge, required) {
let from = edge.from
if (from.installLinks && from.linksIn.size) {
from = from.linksIn.values().next().value
}
// create a virtual root node with the same deps as the node that
// is requesting this one, so that we can get all the peer deps in
// a context where they're likely to be resolvable.
// Note that the virtual root will also have virtual copies of the
// targets of any child Links, so that they resolve appropriately.
const parent = parent_ || this[_virtualRoot](edge.from)
const parent = parent_ || this[_virtualRoot](from)

const spec = npa.resolve(edge.name, edge.spec, edge.from.path)
const spec = npa.resolve(edge.name, edge.spec, from.path)
const first = await this[_nodeFromSpec](edge.name, spec, parent, edge)

// we might have a case where the parent has a peer dependency on
Expand Down Expand Up @@ -1110,11 +1115,10 @@ This is a one-time fix-up, please be patient...
legacyPeerDeps: this.legacyPeerDeps,
overrides: node.overrides,
})

// also need to set up any targets from any link deps, so that
// they are properly reflected in the virtual environment
for (const child of node.children.values()) {
if (child.isLink) {
if (child.isLink && (child.isWorkspace || !child.installLinks)) {
new Node({
path: child.realpath,
sourceReference: child.target,
Expand Down Expand Up @@ -1228,8 +1232,8 @@ This is a one-time fix-up, please be patient...
const isWorkspace = this.idealTree.workspaces && this.idealTree.workspaces.has(spec.name)

// spec is a directory, link it unless installLinks is set or it's a workspace
if (spec.type === 'directory' && (isWorkspace || !installLinks)) {
return this[_linkFromSpec](name, spec, parent, edge)
if (spec.type === 'directory') {
return this[_linkFromSpec](name, spec, parent)
}

// if the spec matches a workspace name, then see if the workspace node will
Expand Down Expand Up @@ -1263,7 +1267,7 @@ This is a one-time fix-up, please be patient...
})
}

[_linkFromSpec] (name, spec, parent, edge) {
[_linkFromSpec] (name, spec, parent) {
const realpath = spec.fetchSpec
const { installLinks, legacyPeerDeps } = this
return rpj(realpath + '/package.json').catch(() => ({})).then(pkg => {
Expand Down Expand Up @@ -1410,6 +1414,29 @@ This is a one-time fix-up, please be patient...
continue
}

// XXX: maybe we need to replace the link with its target
// at this stage so it gets treated as a regular node at
// reified correctly inside node_modules?
// if (link.installLinks && !link.isWorkspace) {
// // console.log(link)edg
// // console.log(link.name)
// // console.log(link.parent)
// // console.log(link.path)
// // console.log(link.realpath)
// // console.log(link.pkg)
// const target = link.target
// link.replaceWith(target)
// this.addTracker('idealTree', target.name, target.location)
// this[_depsQueue].push(target)
// continue
// }
// if installLinks is set then we want to install deps no matter what
if (link.installLinks) {
this.addTracker('idealTree', link.target.name, link.target.location)
this[_depsQueue].push(link.target)
continue
}

const tree = this.idealTree.target
const external = !link.target.isDescendantOf(tree)

Expand Down
3 changes: 2 additions & 1 deletion workspaces/arborist/lib/arborist/reify.js
Original file line number Diff line number Diff line change
Expand Up @@ -668,7 +668,7 @@ module.exports = cls => class Reifier extends cls {
const nm = resolve(node.parent.path, 'node_modules')
await this[_validateNodeModules](nm)

if (node.isLink) {
if (node.isLink && (node.isWorkspace || !node.installLinks)) {
await rm(node.path, { recursive: true, force: true })
await this[_symlink](node)
} else {
Expand All @@ -687,6 +687,7 @@ module.exports = cls => class Reifier extends cls {
Arborist: this.constructor,
resolved: node.resolved,
integrity: node.integrity,
where: dirname(node.path),
})
}
}
Expand Down
1 change: 1 addition & 0 deletions workspaces/arborist/lib/dep-valid.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ const depValid = (child, requested, requestor) => {

const linkValid = (child, requested, requestor) => {
const isLink = !!child.isLink

// if we're installing links and the node is a link, then it's invalid because we want
// a real node to be there. Except for workspaces. They are always links.
if (requestor.installLinks && !child.isWorkspace) {
Expand Down
17 changes: 16 additions & 1 deletion workspaces/arborist/lib/diff.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
// for a given branch of the tree being mutated.

const { depth } = require('treeverse')
const { existsSync } = require('fs')
const { existsSync, lstatSync } = require('fs')

const ssri = require('ssri')

Expand Down Expand Up @@ -114,6 +114,21 @@ const getAction = ({ actual, ideal }) => {
return ideal.inDepBundle ? null : 'ADD'
}

if (actual.isLink) {
let stat
try {
stat = lstatSync(actual.path)
} catch {}

if (stat) {
if (ideal.installLinks && stat.isSymbolicLink()) {
return 'CHANGE'
} else if (!ideal.installLinks && !stat.isSymbolicLink()) {
return 'CHANGE'
}
}
}

// always ignore the root node
if (ideal.isRoot && actual.isRoot) {
return null
Expand Down