fix: honor explicit prefix

[jameschensmith]

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 to tag. This now resolves correctly to range. When resolving the variable range shortly after, though, existing checks are now truthy. Before, it was short-circuited as falsey due to type. 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's package.json rather than prefixRange.

This request addresses the false semver subset check by checking if spec is *. * should still resolve in a package.json if child.version is 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.

[jameschensmith]

If this is the route to go, thoughts on switching these checks for speed?

Suggested change

	const range = !isRange || subset(prefixRange, spec, { loose: true }) || spec === '*'
	const range = !isRange || spec === '*' || subset(prefixRange, spec, { loose: true })

[jameschensmith]

Before merging this PR, I would love for the subset fix to be resolved in node-semver. This would remove the || spec === '*' logic. I created an issue for it (npm/node-semver#374), and will be submitting a PR shortly. The PR is now up (npm/node-semver#375).

[isaacs]

Yeah, this looks weird. Probably shouldn't end up with the name in there...

[isaacs]

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.

[isaacs]

Ah, ok, the || spec === '*' part is unnecessary, but now that npm/node-semver#377 has shipped, I think the other bit looks correct.

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

[jameschensmith]

Ah, ok, the || spec === '*' part is unnecessary, but now that npm/node-semver#377 has shipped, I think the other bit looks correct.

This patch seems like it fixes the problem, and is pretty close to what you put here. Want to take a look? gist.github.com/isaacs/7d42bb2672905d3b27e70bc4015ca135

Yes! That's what I was planning to do after the patch in node-semver (i.e. remove the || spec === '*' and bump semver; the additions to the test apart from what I did are great as well!). 😁👏 If you want me to finish the PR up, let me know. Otherwise, let's get this patched! 🙌

[jameschensmith]

Went ahead and addressed the requested changes. Very straightforward 😊 Thanks for your reviews today!

[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!