Fix: bug in semver check on installProjectDependencies #6426
Conversation
🦋 Changeset detectedLatest commit: 83c0156 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| packageJson: { | ||
| name: "test", | ||
| version: "0.0.1", | ||
| devDependencies: { hardhat: "^3.0.0-next.0" }, // <-- required version |
There was a problem hiding this comment.
When reading through it for the first time, I started worrying and double-checking if we were going to have to update the version here with every release. Now I know we don't, but I guess we could still consider using clearly fake dependency to avoid that momentary confusion for others in the future. Something like:
| devDependencies: { hardhat: "^3.0.0-next.0" }, // <-- required version | |
| devDependencies: { fake-dependency: "^1.2.3-dev" }, // <-- required version |
It's all correct so I'm not requesting this change definitively. Just putting it out there to consider if you want to.
| !semver.satisfies(version, workspaceVersion) && | ||
| !semver.intersects(version, workspaceVersion) | ||
| !semver.satisfies(workspaceVersion, version) && | ||
| !semver.intersects(workspaceVersion, version) |
There was a problem hiding this comment.
Why does this test include intersects? Why not just satisfies?
There was a problem hiding this comment.
That's in case the workspaceVersion (the one that user has currently defined in their package) is a range rather than an exact version.
However, now I wonder why can't we use just intersects. I would assume that if the workspaceVersion is a single version rather than a range, the intersection with a range should still work. Let me double-check that.
There was a problem hiding this comment.
OK, so the intersects works only for ranges and a specific version is not considered a range.
There was a problem hiding this comment.
But is this a safe move? Could I not have an installed version say >= 1.0.0 and a required version of >= 1.2.3 and these would intersect - but we would say that we should update the installed version in this case?
Is it not that we want to ensure that the installed version range is a subset of the required range:
subset(subRange, superRange): Return true if the subRange range is entirely contained by the superRange range.
There was a problem hiding this comment.
That's a great point! We didn't account for the workspace version being "too high" in the initial implementation of this function.
I went ahead and refactored the logic here a bit to be able to better test in what scenario an update should happen.
See #6446
Feel free to merge if you think that's useful.
Use an example that clarifies the reason for the test, unconnected with any implication around the hardhat package.
The parameter order was inverted when calling
semver.satisfies()andsemver.intersects()oninstallProjectDependenciesCloses #6323