Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: bug in semver check on installProjectDependencies #6426

Merged
merged 5 commits into from
Mar 4, 2025
Merged

Fix: bug in semver check on installProjectDependencies #6426

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7c8b883
Fix: bug in semver check on installProjectDependencies
antico5 Feb 27, 2025
a238510
Merge branch 'v-next' into hardhat-init-reinstall
galargh Feb 28, 2025
2e1422f
test: tweak test example
kanej Mar 4, 2025
e5d4453
chore: add a changeset for the fix
kanej Mar 4, 2025
83c0156
fix: test the shouldUpdateDependency helper thoroughly (#6446)
galargh Mar 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions v-next/hardhat/src/internal/cli/init/init.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,8 +484,8 @@ export async function installProjectDependencies(
const workspaceVersion = workspaceDependencies[name];
return (
workspaceVersion !== undefined &&
!semver.satisfies(version, workspaceVersion) &&
!semver.intersects(version, workspaceVersion)
!semver.satisfies(workspaceVersion, version) &&
!semver.intersects(workspaceVersion, version)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this test include intersects? Why not just satisfies?

@galargh

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so the intersects works only for ranges and a specific version is not considered a range.

Copy link
Member

@kanej kanej Mar 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

https://www.npmjs.com/package/semver#ranges-1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

);
})
.map(([name, version]) => `${name}@${version}`);
Expand Down
67 changes: 67 additions & 0 deletions v-next/hardhat/test/internal/cli/init/init.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import type { Template } from "../../../../src/internal/cli/init/template.js";
import type { PackageJson } from "@nomicfoundation/hardhat-utils/package";

import assert from "node:assert/strict";
Expand Down Expand Up @@ -311,6 +312,72 @@ describe("installProjectDependencies", async () => {
}
},
);

it(
"should not update dependencies if they are up-to-date and the user opts-in to the update (specific version)",
{
skip: process.env.HARDHAT_DISABLE_SLOW_TESTS === "true",
},
async () => {
const template: Template = {
name: "test",
packageJson: {
name: "test",
version: "0.0.1",
devDependencies: { hardhat: "^3.0.0-next.0" }, // <-- required version
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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.

},
path: process.cwd(),
files: [],
};

await writeUtf8File(
"package.json",
JSON.stringify({
type: "module",
devDependencies: { hardhat: "3.0.0-next.0" }, // <-- specific version
}),
);
await installProjectDependencies(process.cwd(), template, false, true);

assert.ok(
!(await exists("node_modules")),
"no modules should have been installed",
);
},
);

it(
"should not update dependencies if they are up-to-date and the user opts-in to the update (version range)",
{
skip: process.env.HARDHAT_DISABLE_SLOW_TESTS === "true",
},
async () => {
const template: Template = {
name: "test",
packageJson: {
name: "test",
version: "0.0.1",
devDependencies: { hardhat: "^3.0.0-next.0" }, // <-- required version
},
path: process.cwd(),
files: [],
};

await writeUtf8File(
"package.json",
JSON.stringify({
type: "module",
devDependencies: { hardhat: ">= 3.0.0-next.0" }, // <-- version range
}),
);
await installProjectDependencies(process.cwd(), template, false, true);

assert.ok(
!(await exists("node_modules")),
"no modules should have been installed",
);
},
);
});

describe("initHardhat", async () => {
Expand Down
Loading