From 60a688d964d8b21e02248a33f27c863d0ce4ae87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Caleb=20=E3=83=84=20Everett?= Date: Tue, 16 Nov 2021 14:30:30 -0800 Subject: [PATCH 1/2] feat: omit resolved from registry dependencies 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`. --- .../arborist/lib/arborist/build-ideal-tree.js | 2 + .../arborist/lib/arborist/load-actual.js | 2 + .../arborist/lib/arborist/load-virtual.js | 1 + workspaces/arborist/lib/node.js | 12 +++ workspaces/arborist/lib/override-resolves.js | 11 +++ workspaces/arborist/lib/shrinkwrap.js | 31 +++++-- workspaces/arborist/test/node.js | 23 +++++ workspaces/arborist/test/shrinkwrap.js | 88 +++++++++++++++++++ 8 files changed, 163 insertions(+), 7 deletions(-) create mode 100644 workspaces/arborist/lib/override-resolves.js diff --git a/workspaces/arborist/lib/arborist/build-ideal-tree.js b/workspaces/arborist/lib/arborist/build-ideal-tree.js index dffcd546b8677..c51f53c171ddc 100644 --- a/workspaces/arborist/lib/arborist/build-ideal-tree.js +++ b/workspaces/arborist/lib/arborist/build-ideal-tree.js @@ -310,6 +310,7 @@ module.exports = cls => class IdealTreeBuilder extends cls { ? Shrinkwrap.reset({ path: this.path, lockfileVersion: this.options.lockfileVersion, + resolveOptions: this.options, }).then(meta => Object.assign(root, { meta })) : this.loadVirtual({ root })) @@ -354,6 +355,7 @@ module.exports = cls => class IdealTreeBuilder extends cls { const meta = new Shrinkwrap({ path: this.path, lockfileVersion: this.options.lockfileVersion, + resolveOptions: this.options, }) meta.reset() root.meta = meta diff --git a/workspaces/arborist/lib/arborist/load-actual.js b/workspaces/arborist/lib/arborist/load-actual.js index 0d260858d81c6..087a038c4fd26 100644 --- a/workspaces/arborist/lib/arborist/load-actual.js +++ b/workspaces/arborist/lib/arborist/load-actual.js @@ -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 @@ -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 }) diff --git a/workspaces/arborist/lib/arborist/load-virtual.js b/workspaces/arborist/lib/arborist/load-virtual.js index 4d65e3da6f683..45d1a047a7cc9 100644 --- a/workspaces/arborist/lib/arborist/load-virtual.js +++ b/workspaces/arborist/lib/arborist/load-virtual.js @@ -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') diff --git a/workspaces/arborist/lib/node.js b/workspaces/arborist/lib/node.js index 45c288bcf6cf7..f8b4964ff3778 100644 --- a/workspaces/arborist/lib/node.js +++ b/workspaces/arborist/lib/node.js @@ -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 diff --git a/workspaces/arborist/lib/override-resolves.js b/workspaces/arborist/lib/override-resolves.js new file mode 100644 index 0000000000000..794b2c335dc62 --- /dev/null +++ b/workspaces/arborist/lib/override-resolves.js @@ -0,0 +1,11 @@ +function overrideResolves (resolved, opts = {}) { + const { omitLockfileRegistryResolved = false } = opts + + if (omitLockfileRegistryResolved) { + return undefined + } + + return resolved +} + +module.exports = { overrideResolves } diff --git a/workspaces/arborist/lib/shrinkwrap.js b/workspaces/arborist/lib/shrinkwrap.js index b45fea0ac6111..b1ea2cacb360e 100644 --- a/workspaces/arborist/lib/shrinkwrap.js +++ b/workspaces/arborist/lib/shrinkwrap.js @@ -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 => { @@ -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), @@ -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 } @@ -331,6 +337,7 @@ class Shrinkwrap { hiddenLockfile = false, log = procLog, lockfileVersion, + resolveOptions = {}, } = options this.lockfileVersion = hiddenLockfile ? 3 @@ -349,6 +356,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 } @@ -827,7 +835,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 @@ -883,7 +891,10 @@ 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 () { @@ -891,7 +902,10 @@ class Shrinkwrap { 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 @@ -902,7 +916,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()) { @@ -1010,7 +1027,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) { diff --git a/workspaces/arborist/test/node.js b/workspaces/arborist/test/node.js index ecdf72c22a6dc..29f23aafba3c2 100644 --- a/workspaces/arborist/test/node.js +++ b/workspaces/arborist/test/node.js @@ -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) +}) diff --git a/workspaces/arborist/test/shrinkwrap.js b/workspaces/arborist/test/shrinkwrap.js index f752c724a35e7..5d23904fcaee2 100644 --- a/workspaces/arborist/test/shrinkwrap.js +++ b/workspaces/arborist/test/shrinkwrap.js @@ -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 From 802732366316beea16bc6e8cff9f793f98dc050f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Caleb=20=E3=83=84=20Everett?= Date: Wed, 5 Jan 2022 14:28:23 -0800 Subject: [PATCH 2/2] feat: add record default registry option Create shrinkwrap files with resolved urls modified to replace the configured registry with the default registry, https://registry.npmjs.org. The default registry is a magic value meaning the current registry, so recording resolved with the default registry allows users to switch to a different registry without removing their lockfile. The path portion of the acutal resolved url is preserved so this trick only works when the different registries host tarballs at the same relative paths. It's faster than the omitLockfileRegistryResolved option because npm doesn't need to fetch each pacument to resolve the tarball url. --- workspaces/arborist/lib/override-resolves.js | 47 ++++++++++++++++++-- workspaces/arborist/lib/shrinkwrap.js | 11 ++--- workspaces/arborist/test/shrinkwrap.js | 31 ++++++++++++- 3 files changed, 76 insertions(+), 13 deletions(-) diff --git a/workspaces/arborist/lib/override-resolves.js b/workspaces/arborist/lib/override-resolves.js index 794b2c335dc62..f991e6d962389 100644 --- a/workspaces/arborist/lib/override-resolves.js +++ b/workspaces/arborist/lib/override-resolves.js @@ -1,10 +1,51 @@ -function overrideResolves (resolved, opts = {}) { - const { omitLockfileRegistryResolved = false } = opts +const npa = require('npm-package-arg') - if (omitLockfileRegistryResolved) { +function overrideResolves (node, resolved, opts = {}) { + const { + omitLockfileRegistryResolved = false, + recordDefaultRegistry = false, + } = opts + + const isRegistryDependency = node.isRegistryDependency + + // omit the resolved url of registry dependencies. this makes installs slower + // because npm must resolve the url for each package version but it allows + // users to use a lockfile across registries that host tarballs at different + // paths. + if (isRegistryDependency && omitLockfileRegistryResolved) { return undefined } + // replace the configured registry with the default registry. the default + // registry is a magic value meaning the current registry, so recording + // resolved with the default registry allows users to switch to a + // different registry without removing their lockfile. The path portion + // of the resolved url is preserved so this trick only works when the + // different registries host tarballs at the same relative paths. + if (isRegistryDependency && recordDefaultRegistry) { + const scope = npa(node.packageName).scope + const registry = scope && opts[`${scope}:registry`] + ? opts[`${scope}:registry`] + : opts.registry + + // normalize registry url - strip trailing slash. + // TODO improve normalization for both the configured registry and resolved + // url. consider port, protocol, more path normalization. + const normalized = registry.endsWith('/') + ? registry.slice(0, -1) + : registry + + // only replace the host if the resolved url is for the configured + // registry. registries may host tarballs on another server. return + // undefined so npm will re-resolve the url from the current registry when + // it reads the lockfile. + if (resolved.startsWith(normalized)) { + return 'https://registry.npmjs.org' + resolved.slice(normalized.length) + } else { + return undefined + } + } + return resolved } diff --git a/workspaces/arborist/lib/shrinkwrap.js b/workspaces/arborist/lib/shrinkwrap.js index b1ea2cacb360e..f8dd056d62bee 100644 --- a/workspaces/arborist/lib/shrinkwrap.js +++ b/workspaces/arborist/lib/shrinkwrap.js @@ -300,13 +300,8 @@ class Shrinkwrap { }) const resolved = consistentResolve(node.resolved, node.path, path, true) - // hide resolved from registry dependencies. - if (!resolved) { - // no-op - } else if (node.isRegistryDependency) { - meta.resolved = overrideResolves(resolved, options) - } else { - meta.resolved = resolved + if (resolved) { + meta.resolved = overrideResolves(node, resolved, options) } if (node.extraneous) { @@ -1027,7 +1022,7 @@ class Shrinkwrap { spec.type !== 'git' && spec.type !== 'file' && spec.type !== 'remote') { - lock.resolved = overrideResolves(node.resolved, this.resolveOptions) + lock.resolved = overrideResolves(node, node.resolved, this.resolveOptions) } if (node.integrity) { diff --git a/workspaces/arborist/test/shrinkwrap.js b/workspaces/arborist/test/shrinkwrap.js index 5d23904fcaee2..daa661ee38e08 100644 --- a/workspaces/arborist/test/shrinkwrap.js +++ b/workspaces/arborist/test/shrinkwrap.js @@ -247,7 +247,7 @@ t.only('resolveOptions', async t => { dependencies: { registry: '^1.0.0', 'some-other-registry': '^1.0.0', - '@scoped/some-other-registry': '^1.0.0', + '@scope/some-package': '^1.0.0', tar: url, }, }, @@ -271,7 +271,7 @@ t.only('resolveOptions', async t => { }) const scopedOtherRegistry = new Node({ - pkg: { name: '@scope/some-other-registry', version: '1.2.3' }, + pkg: { name: '@scope/some-package', version: '1.2.3' }, resolved: someOtherRegistry, integrity: 'sha512-registry', parent: root, @@ -312,6 +312,33 @@ t.only('resolveOptions', async t => { t.strictSame(data.dependencies.tar.resolved, undefined) }) + await t.test('recordDefaultRegistry: true', async t => { + const { data } = await getData({ + recordDefaultRegistry: true, + registry: 'https://private.registry.org/deadbeef', + '@scope:registry': 'https://someother.registry.org', + }) + + // unscoped packages that resolve to their configured registry should be + // record the default registry + t.strictSame(data.packages['node_modules/registry'].resolved, + 'https://registry.npmjs.org/registry/-/registry-1.2.3.tgz') + t.strictSame(data.dependencies.registry.resolved, + 'https://registry.npmjs.org/registry/-/registry-1.2.3.tgz') + + // scoped packages that resolve to their configured registry should be + // record the default registry + t.strictSame(data.packages['node_modules/@scope/some-package'].resolved, + 'https://registry.npmjs.org/registry/-/registry-1.2.3.tgz') + t.strictSame(data.dependencies['@scope/some-package'].resolved, + 'https://registry.npmjs.org/registry/-/registry-1.2.3.tgz') + + // packages with resolved urls that don't match the configured registry + // should record undefined so npm resolves their url again. + t.strictSame(data.packages['node_modules/some-other-registry'].resolved, undefined) + t.strictSame(data.dependencies['some-other-registry'].resolved, undefined) + }) + t.test('metaFromNode default', async t => { // test to cover options default. const { registry } = await getData(undefined)