fix: honor explicit prefix #254
Conversation
lib/arborist/reify.js
Outdated
| @@ -905,7 +905,7 @@ module.exports = cls => class Reifier extends cls { | |||
| // would allow versions outside the requested range. Tags and | |||
| // specific versions save with the save-prefix. | |||
| const isRange = (subSpec || req).type === 'range' | |||
| const range = !isRange || subset(prefixRange, spec, { loose: true }) | |||
| const range = !isRange || subset(prefixRange, spec, { loose: true }) || spec === '*' | |||
There was a problem hiding this comment.
If this is the route to go, thoughts on switching these checks for speed?
| const range = !isRange || subset(prefixRange, spec, { loose: true }) || spec === '*' | |
| const range = !isRange || spec === '*' || subset(prefixRange, spec, { loose: true }) |
|
Before merging this PR, I would love for the subset fix to be resolved in node-semver. This would remove the |
| @@ -808,7 +808,7 @@ t.test('saving the ideal tree', t => { | |||
| dependencies: { | |||
| a: 'git+ssh://git@github.com:foo/bar#baz', | |||
| b: '', | |||
| d: 'd@npm:c@1.x <1.9.9', | |||
There was a problem hiding this comment.
Yeah, this looks weird. Probably shouldn't end up with the name in there...
There was a problem hiding this comment.
So, I don't think this is quite the right way to go. I might be wrong, and I agree that there is a bug here, but it'd be much easier to evaluate if we start from a failing test (such as the case you suggested), and then figure out exactly where it's going wrong and how to make that test pass.
I'll dig in today and try to get more insight. At the very least, saving the name on the right-hand-side of the dependencies field is definitely a bug, that shouldn't be happening.
|
Ah, ok, the This patch seems like it fixes the problem, and is pretty close to what you put here. Want to take a look? https://gist.github.com/isaacs/7d42bb2672905d3b27e70bc4015ca135 |
Yes! That's what I was planning to do after the patch in node-semver (i.e. remove the |
|
Went ahead and addressed the requested changes. Very straightforward 😊 Thanks for your reviews today! |
Honors explicit prefix. Signed-off-by: James Chen-Smith <jameschensmith@gmail.com> PR-URL: #254 Credit: @jameschensmith Close: #254 Reviewed-by: @isaacs
|
This is shipped now! Will be in next npm release. I shuffled around the commit history a bit so that semver update is in one, and the functional updates come together after it. Thanks! |
Summary
Honors explicit prefix. npm-package-arg was being used incorrectly in reify when saving the ideal tree. Only the spec was being passed in, which resulted in some unusual results (submitted a ticket to address that in npm-package-arg as well).
Once this was addressed, another issue was spotted, where passing no version on a new package results in
*being added. Previously, the type resolved totag. This now resolves correctly torange. When resolving the variablerangeshortly after, though, existing checks are now truthy. Before, it was short-circuited as falsey due totype. Also, node-semver returns false when comparing subsets of*other than itself (it's documented in the code). The now falsey resolution makes spec(*) used in a user'spackage.jsonrather thanprefixRange.This request addresses the false semver subset check by checking if
specis*.*should still resolve in apackage.jsonifchild.versionis falsy for any reason.In addition, there was a test which had an unusual alias. This fix brought that abnormality to light. This request corrects that test.
References
Fixes #253.