feat: implement overrides #354
Conversation
nlf
force-pushed
the
nlf/overrides-support
branch
from
5dddc29
to
630e3f6
Compare
this allows a user to specify an override as $packageName where 'packageName' is a direct dependency of the root package and that reference will be resolved to the range of the referenced dep
nlf
force-pushed
the
nlf/overrides-support
branch
from
630e3f6
to
ac31ecc
Compare
|
|
||
| const fooBazEdge = bazEdge.to.edgesOut.get('foo') | ||
| t.equal(fooBazEdge.valid, true, 'cyclical foo is valid') | ||
| t.equal(fooBazEdge.to.version, '2.0.0', 'override broke the cycle') |
There was a problem hiding this comment.
way to break the cycle, buddy!
| ...(edgeTo ? { to: edgeTo } : {}), | ||
| ...(edge.error ? { error: edge.error } : {}), | ||
| ...(edge.peerConflicted ? { peerConflicted: true } : {}), | ||
| ...(override ? { overridden: override } : {}), |
There was a problem hiding this comment.
fwiw, this would be ever so slightly more performant:
| ...(edgeTo ? { to: edgeTo } : {}), | |
| ...(edge.error ? { error: edge.error } : {}), | |
| ...(edge.peerConflicted ? { peerConflicted: true } : {}), | |
| ...(override ? { overridden: override } : {}), | |
| ...(edgeTo && { to: edgeTo }), | |
| ...(edge.error && { error: edge.error }), | |
| ...(edge.peerConflicted && { peerConflicted: true }), | |
| ...(override && { overridden: override }), |
but obv is a bit unrelated to this PR.
There was a problem hiding this comment.
good note, but yes unrelated. arborist in general could use some refactoring for things like this
| ...(this.rawSpec !== this.spec ? { | ||
| rawSpec: this.rawSpec, | ||
| overridden: true, | ||
| } : {}), |
There was a problem hiding this comment.
| ...(this.rawSpec !== this.spec ? { | |
| rawSpec: this.rawSpec, | |
| overridden: true, | |
| } : {}), | |
| ...(this.rawSpec !== this.spec && { | |
| rawSpec: this.rawSpec, | |
| overridden: true, | |
| }), |
| if (Object.keys(overrides).length > 0) { | ||
| this.overrides = new OverrideSet({ | ||
| overrides: this[_package].overrides, | ||
| }) | ||
| } |
There was a problem hiding this comment.
does this mean that overrides will not always be present? it seems easier to deal with if it's always an OverrideSet, just sometimes empty.
There was a problem hiding this comment.
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
| 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.
this seems like a strange sentinel/prefix approach. are you envisioning that an override can't ever start with $, even tho that might be a legitimate bundledDep folder name inside node_modules?
There was a problem hiding this comment.
npm-package-arg defines $ as an invalid character in a package name, anywhere we attempt to parse a spec will explode. i'd be surprised if it worked as a bundled dep name, have you seen it work that way?
i chose the $ prefix specifically because npm-package-arg will throw when encountering it, but if that assumption doesn't hold water then we'll definitely have to come up with something else
There was a problem hiding this comment.
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.
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.
| }) | ||
|
|
||
| t.test('overrides a nested dependency that also exists as a direct dependency', async (t) => { | ||
| generateNocks(t, { |
There was a problem hiding this comment.
You use this same Nock fixture quite a bit. It could be done once and referenced.
There was a problem hiding this comment.
i liked keeping the calls to generateNocks inline in the test, it's handy to have the context of what a package's dependency tree looks like right in the test that exercises it. part of the struggles with the current testing that i experienced was having to go read fixtures to find out what the dep tree was even supposed to be.
it's a first pass at introducing nock here though, and i'd wager there's lots of improvements to be made
| @@ -1,5 +1,6 @@ | |||
| const util = require('util') | |||
There was a problem hiding this comment.
This entire thing is just one test with a lot of assertions. Is this how we want to test data component classes?
There was a problem hiding this comment.
lol it is not. i deemed it out of scope, however, to completely refactor the existing tests so i chose to add my tests in the same pattern as what existed and plan on a future refactor
|
Thanks a lot for implementing this! |
This pull request implements overrides as described in the spec
There are a couple of todo notes for myself in the code that I'm going through my notes to find context on, so this is a draft until I get those sorted out.I wasn't able to figure out what the notes were meant to address. Since tests tell a positive story here, I'm leaving the comments as they are in case of future issues and this is ready for review