-
Notifications
You must be signed in to change notification settings - Fork 63
feat: implement overrides #354
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -29,6 +29,7 @@ class ArboristEdge {} | |||||||||||||||||
const printableEdge = (edge) => { | ||||||||||||||||||
const edgeFrom = edge.from && edge.from.location | ||||||||||||||||||
const edgeTo = edge.to && edge.to.location | ||||||||||||||||||
const override = edge.overrides && edge.overrides.value | ||||||||||||||||||
|
||||||||||||||||||
return Object.assign(new ArboristEdge(), { | ||||||||||||||||||
name: edge.name, | ||||||||||||||||||
|
@@ -38,12 +39,13 @@ const printableEdge = (edge) => { | |||||||||||||||||
...(edgeTo ? { to: edgeTo } : {}), | ||||||||||||||||||
...(edge.error ? { error: edge.error } : {}), | ||||||||||||||||||
...(edge.peerConflicted ? { peerConflicted: true } : {}), | ||||||||||||||||||
...(override ? { overridden: override } : {}), | ||||||||||||||||||
}) | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
class Edge { | ||||||||||||||||||
constructor (options) { | ||||||||||||||||||
const { type, name, spec, accept, from } = options | ||||||||||||||||||
const { type, name, spec, accept, from, overrides } = options | ||||||||||||||||||
|
||||||||||||||||||
if (typeof spec !== 'string') { | ||||||||||||||||||
throw new TypeError('must provide string spec') | ||||||||||||||||||
|
@@ -55,6 +57,10 @@ class Edge { | |||||||||||||||||
|
||||||||||||||||||
this[_spec] = spec | ||||||||||||||||||
|
||||||||||||||||||
if (overrides !== undefined) { | ||||||||||||||||||
this.overrides = overrides | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
if (accept !== undefined) { | ||||||||||||||||||
if (typeof accept !== 'string') { | ||||||||||||||||||
throw new TypeError('accept field must be a string if provided') | ||||||||||||||||||
|
@@ -82,8 +88,11 @@ class Edge { | |||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
satisfiedBy (node) { | ||||||||||||||||||
return node.name === this.name && | ||||||||||||||||||
depValid(node, this.spec, this.accept, this.from) | ||||||||||||||||||
if (node.name !== this.name) { | ||||||||||||||||||
return false | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
return depValid(node, this.spec, this.accept, this.from) | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
explain (seen = []) { | ||||||||||||||||||
|
@@ -101,6 +110,10 @@ class Edge { | |||||||||||||||||
type: this.type, | ||||||||||||||||||
name: this.name, | ||||||||||||||||||
spec: this.spec, | ||||||||||||||||||
...(this.rawSpec !== this.spec ? { | ||||||||||||||||||
rawSpec: this.rawSpec, | ||||||||||||||||||
overridden: true, | ||||||||||||||||||
} : {}), | ||||||||||||||||||
Comment on lines
+113
to
+116
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
...(bundled ? { bundled } : {}), | ||||||||||||||||||
...(error ? { error } : {}), | ||||||||||||||||||
...(from ? { from: from.explain(null, seen) } : {}), | ||||||||||||||||||
|
@@ -143,7 +156,28 @@ class Edge { | |||||||||||||||||
return this[_name] | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
get rawSpec () { | ||||||||||||||||||
return this[_spec] | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
get spec () { | ||||||||||||||||||
if (this.overrides && this.overrides.value && this.overrides.name === this.name) { | ||||||||||||||||||
if (this.overrides.value.startsWith('$')) { | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this seems like a strange sentinel/prefix approach. are you envisioning that an override can't ever start with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. npm-package-arg defines i chose the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nope! just thinking about possibilities. Will every prior version of npa throw on it? It's a fine assumption, I'm just not sure why a stringly typed interface is the ideal choice. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. afaict all version of npa will throw on it. using a string was a gut call. there are most definitely other approaches we could use here, but this was simple to parse and i knew it would be invalid as a "normal" package spec which is what i wanted. |
||||||||||||||||||
const ref = this.overrides.value.slice(1) | ||||||||||||||||||
const pkg = this.from.root.package | ||||||||||||||||||
const overrideSpec = (pkg.devDependencies && pkg.devDependencies[ref]) || | ||||||||||||||||||
(pkg.optionalDependencies && pkg.optionalDependencies[ref]) || | ||||||||||||||||||
(pkg.dependencies && pkg.dependencies[ref]) || | ||||||||||||||||||
(pkg.peerDependencies && pkg.peerDependencies[ref]) | ||||||||||||||||||
|
||||||||||||||||||
if (overrideSpec) { | ||||||||||||||||||
return overrideSpec | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
throw new Error(`Unable to resolve reference ${this.overrides.value}`) | ||||||||||||||||||
} | ||||||||||||||||||
return this.overrides.value | ||||||||||||||||||
} | ||||||||||||||||||
return this[_spec] | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
|
@@ -213,6 +247,7 @@ class Edge { | |||||||||||||||||
if (node.edgesOut.has(this.name)) { | ||||||||||||||||||
node.edgesOut.get(this.name).detach() | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
node.addEdgeOut(this) | ||||||||||||||||||
this.reload() | ||||||||||||||||||
} | ||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,6 +32,7 @@ const semver = require('semver') | |
const nameFromFolder = require('@npmcli/name-from-folder') | ||
const Edge = require('./edge.js') | ||
const Inventory = require('./inventory.js') | ||
const OverrideSet = require('./override-set.js') | ||
const { normalize } = require('read-package-json-fast') | ||
const { getPaths: getBinPaths } = require('bin-links') | ||
const npa = require('npm-package-arg') | ||
|
@@ -88,6 +89,8 @@ class Node { | |
legacyPeerDeps = false, | ||
linksIn, | ||
hasShrinkwrap, | ||
overrides, | ||
loadOverrides = false, | ||
extraneous = true, | ||
dev = true, | ||
optional = true, | ||
|
@@ -190,6 +193,17 @@ class Node { | |
// because this.package is read when adding to inventory | ||
this[_package] = pkg && typeof pkg === 'object' ? pkg : {} | ||
|
||
if (overrides) { | ||
this.overrides = overrides | ||
} else if (loadOverrides) { | ||
const overrides = this[_package].overrides || {} | ||
if (Object.keys(overrides).length > 0) { | ||
this.overrides = new OverrideSet({ | ||
overrides: this[_package].overrides, | ||
}) | ||
} | ||
Comment on lines
+200
to
+204
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this mean that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was largely due to the nature of tests written in arborist. adding an empty OverrideSet to every node regardless of if overrides are in use breaks dozens of test snapshots. to avoid churn, i opted to omit the overrides unless they're in use. i agree it's weird though, and i do plan to remove that guard and redo the snapshots in a future pull request where it can be isolated |
||
} | ||
|
||
// only relevant for the root and top nodes | ||
this.meta = meta | ||
|
||
|
@@ -963,6 +977,11 @@ class Node { | |
return false | ||
} | ||
|
||
// XXX need to check for two root nodes? | ||
if (node.overrides !== this.overrides) { | ||
return false | ||
} | ||
|
||
ignorePeers = new Set(ignorePeers) | ||
|
||
// gather up all the deps of this node and that are only depended | ||
|
@@ -1208,6 +1227,10 @@ class Node { | |
this[_changePath](newPath) | ||
} | ||
|
||
if (parent.overrides) { | ||
this.overrides = parent.overrides.getNodeRule(this) | ||
} | ||
|
||
// clobbers anything at that path, resets all appropriate references | ||
this.root = parent.root | ||
} | ||
|
@@ -1279,11 +1302,33 @@ class Node { | |
} | ||
} | ||
|
||
assertRootOverrides () { | ||
if (!this.isProjectRoot || !this.overrides) { | ||
return | ||
} | ||
|
||
for (const edge of this.edgesOut.values()) { | ||
// if these differ an override has been applied, those are not allowed | ||
// for top level dependencies so throw an error | ||
if (edge.spec !== edge.rawSpec && !edge.spec.startsWith('$')) { | ||
throw Object.assign(new Error(`Override for ${edge.name}@${edge.rawSpec} conflicts with direct dependency`), { code: 'EOVERRIDE' }) | ||
} | ||
} | ||
} | ||
|
||
addEdgeOut (edge) { | ||
if (this.overrides) { | ||
edge.overrides = this.overrides.getEdgeRule(edge) | ||
} | ||
|
||
this.edgesOut.set(edge.name, edge) | ||
} | ||
|
||
addEdgeIn (edge) { | ||
if (edge.overrides) { | ||
this.overrides = edge.overrides | ||
} | ||
|
||
this.edgesIn.add(edge) | ||
|
||
// try to get metadata from the yarn.lock file | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw, this would be ever so slightly more performant:
but obv is a bit unrelated to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good note, but yes unrelated. arborist in general could use some refactoring for things like this