Skip to content

Commit

Permalink
feat: omit resolved from registry dependencies
Browse files Browse the repository at this point in the history
Implement `$disable-write-resolves` described in npm/rfcs#486.  I named
the option `omitLockfileRegistryResolved` but that can be changed later.

Put simply, this option causes npm to create lock files without a
`resolved` key for registry dependencies forcing npm to use the current
configured registry and resolve package tarball urls on install. This
fixes install errors when users change registries and the recorded
resolved url is incorrect.

This option causes slower installs because npm must fetch each packages
manifest to find the tarball url, but it's the most comprehensive
solution to this problem. Options like recording always the default
registry, or recording a special 'current registry' sigil will break if
registries host tarballs at different paths. For example
`${REGISTRY}/npm/-/npm-8.3.0.tgz` only works if all registries host
tarballs at `npm/-/npm-8.3.0.tgz`.
  • Loading branch information
Caleb ツ Everett committed Apr 4, 2022
1 parent f37f7d2 commit 7aaa344
Show file tree
Hide file tree
Showing 8 changed files with 163 additions and 7 deletions.
2 changes: 2 additions & 0 deletions workspaces/arborist/lib/arborist/build-ideal-tree.js
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,7 @@ Try using the package name instead, e.g:
? Shrinkwrap.reset({
path: this.path,
lockfileVersion: this.options.lockfileVersion,
resolveOptions: this.options,
}).then(meta => Object.assign(root, { meta }))
: this.loadVirtual({ root }))

Expand Down Expand Up @@ -386,6 +387,7 @@ Try using the package name instead, e.g:
const meta = new Shrinkwrap({
path: this.path,
lockfileVersion: this.options.lockfileVersion,
resolveOptions: this.options,
})
meta.reset()
root.meta = meta
Expand Down
2 changes: 2 additions & 0 deletions workspaces/arborist/lib/arborist/load-actual.js
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ module.exports = cls => class ActualLoader extends cls {
const meta = await Shrinkwrap.load({
path: this[_actualTree].path,
hiddenLockfile: true,
resolveOptions: this.options,
})
if (meta.loadedFromDisk) {
this[_actualTree].meta = meta
Expand All @@ -155,6 +156,7 @@ module.exports = cls => class ActualLoader extends cls {
const meta = await Shrinkwrap.load({
path: this[_actualTree].path,
lockfileVersion: this.options.lockfileVersion,
resolveOptions: this.options,
})
this[_actualTree].meta = meta
return this[_loadActualActually]({ root, ignoreMissing })
Expand Down
1 change: 1 addition & 0 deletions workspaces/arborist/lib/arborist/load-virtual.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ module.exports = cls => class VirtualLoader extends cls {
const s = await Shrinkwrap.load({
path: this.path,
lockfileVersion: this.options.lockfileVersion,
resolveOptions: this.options,
})
if (!s.loadedFromDisk && !options.root) {
const er = new Error('loadVirtual requires existing shrinkwrap file')
Expand Down
12 changes: 12 additions & 0 deletions workspaces/arborist/lib/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,18 @@ class Node {
return this === this.root || this === this.root.target
}

get isRegistryDependency () {
if (this.edgesIn.size === 0) {
return false
}
for (const edge of this.edgesIn) {
if (!npa(edge.spec).registry) {
return false
}
}
return true
}

* ancestry () {
for (let anc = this; anc; anc = anc.resolveParent) {
yield anc
Expand Down
11 changes: 11 additions & 0 deletions workspaces/arborist/lib/override-resolves.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
function overrideResolves (resolved, opts = {}) {
const { omitLockfileRegistryResolved = false } = opts

if (omitLockfileRegistryResolved) {
return undefined
}

return resolved
}

module.exports = { overrideResolves }
31 changes: 24 additions & 7 deletions workspaces/arborist/lib/shrinkwrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ const specFromResolved = resolved => {
const relpath = require('./relpath.js')

const consistentResolve = require('./consistent-resolve.js')
const { overrideResolves } = require('./override-resolves.js')

const maybeReadFile = file => {
return readFile(file, 'utf8').then(d => d, er => {
Expand Down Expand Up @@ -265,7 +266,7 @@ class Shrinkwrap {
return s
}

static metaFromNode (node, path) {
static metaFromNode (node, path, options = {}) {
if (node.isLink) {
return {
resolved: relpath(path, node.realpath),
Expand Down Expand Up @@ -299,7 +300,12 @@ class Shrinkwrap {
})

const resolved = consistentResolve(node.resolved, node.path, path, true)
if (resolved) {
// hide resolved from registry dependencies.
if (!resolved) {
// no-op
} else if (node.isRegistryDependency) {
meta.resolved = overrideResolves(resolved, options)
} else {
meta.resolved = resolved
}

Expand Down Expand Up @@ -330,6 +336,7 @@ class Shrinkwrap {
shrinkwrapOnly = false,
hiddenLockfile = false,
lockfileVersion,
resolveOptions = {},
} = options

this.lockfileVersion = hiddenLockfile ? 3
Expand All @@ -347,6 +354,7 @@ class Shrinkwrap {
this.yarnLock = null
this.hiddenLockfile = hiddenLockfile
this.loadingError = null
this.resolveOptions = resolveOptions
// only load npm-shrinkwrap.json in dep trees, not package-lock
this.shrinkwrapOnly = shrinkwrapOnly
}
Expand Down Expand Up @@ -830,7 +838,7 @@ class Shrinkwrap {
resolved,
integrity,
hasShrinkwrap,
} = Shrinkwrap.metaFromNode(node, this.path)
} = Shrinkwrap.metaFromNode(node, this.path, this.resolveOptions)
node.resolved = node.resolved || resolved || null
node.integrity = node.integrity || integrity || null
node.hasShrinkwrap = node.hasShrinkwrap || hasShrinkwrap || false
Expand Down Expand Up @@ -886,15 +894,21 @@ class Shrinkwrap {
[_updateWaitingNode] (loc) {
const node = this[_awaitingUpdate].get(loc)
this[_awaitingUpdate].delete(loc)
this.data.packages[loc] = Shrinkwrap.metaFromNode(node, this.path)
this.data.packages[loc] = Shrinkwrap.metaFromNode(
node,
this.path,
this.resolveOptions)
}

commit () {
if (this.tree) {
if (this.yarnLock) {
this.yarnLock.fromTree(this.tree)
}
const root = Shrinkwrap.metaFromNode(this.tree.target, this.path)
const root = Shrinkwrap.metaFromNode(
this.tree.target,
this.path,
this.resolveOptions)
this.data.packages = {}
if (Object.keys(root).length) {
this.data.packages[''] = root
Expand All @@ -905,7 +919,10 @@ class Shrinkwrap {
continue
}
const loc = relpath(this.path, node.path)
this.data.packages[loc] = Shrinkwrap.metaFromNode(node, this.path)
this.data.packages[loc] = Shrinkwrap.metaFromNode(
node,
this.path,
this.resolveOptions)
}
} else if (this[_awaitingUpdate].size > 0) {
for (const loc of this[_awaitingUpdate].keys()) {
Expand Down Expand Up @@ -1013,7 +1030,7 @@ class Shrinkwrap {
spec.type !== 'git' &&
spec.type !== 'file' &&
spec.type !== 'remote') {
lock.resolved = node.resolved
lock.resolved = overrideResolves(node.resolved, this.resolveOptions)
}

if (node.integrity) {
Expand Down
23 changes: 23 additions & 0 deletions workspaces/arborist/test/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -2842,3 +2842,26 @@ t.test('overrides', (t) => {

t.end()
})

t.test('node with no edges in is not a registry dep', async t => {
const node = new Node({ path: '/foo' })
t.equal(node.isRegistryDependency, false)
})

t.test('node with non registry edge in is not a registry dep', async t => {
const root = new Node({ path: '/some/path', pkg: { dependencies: { registry: '', tar: '' } } })
const node = new Node({ pkg: { name: 'node', version: '1.0.0' }, parent: root })

new Node({ pkg: { name: 'registry', dependencies: { node: '^1.0.0' } }, parent: root })
new Node({ pkg: { name: 'tar', dependencies: { node: 'file:node' } }, parent: root })

t.equal(node.isRegistryDependency, false)
})

t.test('node with only registry edges in a registry dep', async t => {
const root = new Node({ path: '/some/path', pkg: { dependencies: { registry: '', tar: '' } } })
const node = new Node({ pkg: { name: 'node', version: '1.0.0' }, parent: root })
new Node({ pkg: { name: 'registry', dependencies: { node: '^1.0.0' } }, parent: root })

t.equal(node.isRegistryDependency, true)
})
88 changes: 88 additions & 0 deletions workspaces/arborist/test/shrinkwrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,94 @@ t.test('throws when attempting to access data before loading', t => {
t.end()
})

t.only('resolveOptions', async t => {
const url = 'https://private.registry.org/deadbeef/registry/-/registry-1.2.3.tgz'
const someOtherRegistry = 'https://someother.registry.org/registry/-/registry-1.2.3.tgz'
const getData = async (resolveOptions) => {
const dir = t.testdir()
const meta = await Shrinkwrap.load({
path: dir,
resolveOptions,
})

const root = new Node({
pkg: {
name: 'root',
dependencies: {
registry: '^1.0.0',
'some-other-registry': '^1.0.0',
'@scoped/some-other-registry': '^1.0.0',
tar: url,
},
},
path: dir,
realpath: dir,
meta,
})

const registry = new Node({
pkg: { name: 'registry', version: '1.2.3' },
resolved: url,
integrity: 'sha512-registry',
parent: root,
})

const otherRegistry = new Node({
pkg: { name: 'some-other-registry', version: '1.2.3' },
resolved: someOtherRegistry,
integrity: 'sha512-registry',
parent: root,
})

const scopedOtherRegistry = new Node({
pkg: { name: '@scope/some-other-registry', version: '1.2.3' },
resolved: someOtherRegistry,
integrity: 'sha512-registry',
parent: root,
})

const tar = new Node({
pkg: { name: 'tar', version: '1.2.3' },
resolved: url,
integrity: 'sha512-registry',
parent: root,
})

calcDepFlags(root)
meta.add(root)
return { data: meta.commit(), registry, tar, root, otherRegistry, scopedOtherRegistry }
}

await t.test('omitLockfileRegistryResolved', async t => {
const { data } = await getData({ omitLockfileRegistryResolved: true })
// registry dependencies in v2 packages and v1 dependencies should
// have resolved stripped.
t.strictSame(data.packages['node_modules/registry'].resolved, undefined)
t.strictSame(data.dependencies.registry.resolved, undefined)

// tar should have resolved because it is not a registry dep.
t.strictSame(data.packages['node_modules/tar'].resolved, url)
// v1 url dependencies never have resolved.
t.strictSame(data.dependencies.tar.resolved, undefined)
})

await t.test('omitLockfileRegistryResolved: false', async t => {
const { data } = await getData({ omitLockfileRegistryResolved: false })
t.strictSame(data.packages['node_modules/registry'].resolved, url)
t.strictSame(data.dependencies.registry.resolved, url)

t.strictSame(data.packages['node_modules/tar'].resolved, url)
// v1 url dependencies never have resolved.
t.strictSame(data.dependencies.tar.resolved, undefined)
})

t.test('metaFromNode default', async t => {
// test to cover options default.
const { registry } = await getData(undefined)
t.strictSame(Shrinkwrap.metaFromNode(registry, '').resolved, url)
})
})

t.test('construct metadata from node and package data', t => {
const meta = new Shrinkwrap({ path: '/home/user/projects/root' })
// fake load
Expand Down

0 comments on commit 7aaa344

Please sign in to comment.