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.

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

bugfix: ignore ranges when snapshot versioning is performed (use exact version) #857

Merged
merged 4 commits into from Jul 4, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/stupid-jars-rest.md
@@ -0,0 +1,5 @@
---
"@changesets/apply-release-plan": patch
Andarist marked this conversation as resolved.
Show resolved Hide resolved
---

bugfix: ignore ranges when snapshot versioning is performed (use exact version)
Andarist marked this conversation as resolved.
Show resolved Hide resolved
57 changes: 54 additions & 3 deletions packages/apply-release-plan/src/index.test.ts
Expand Up @@ -74,7 +74,11 @@ async function testSetup(
fixtureName: string,
releasePlan: ReleasePlan,
config?: Config,
setupFunc?: (tempDir: string) => Promise<any>
setupFunc?: (tempDir: string) => Promise<any>,
snapshotConfig?: {
dotansimha marked this conversation as resolved.
Show resolved Hide resolved
snapshot: string | undefined;
useCalculatedVersionForSnapshots: boolean;
dotansimha marked this conversation as resolved.
Show resolved Hide resolved
}
) {
if (!config) {
config = {
Expand All @@ -89,7 +93,8 @@ async function testSetup(
___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH: {
onlyUpdatePeerDependentsWhenOutOfRange: false,
updateInternalDependents: "out-of-range",
useCalculatedVersionForSnapshots: false
useCalculatedVersionForSnapshots:
snapshotConfig?.useCalculatedVersionForSnapshots ?? false
}
};
}
Expand All @@ -108,7 +113,8 @@ async function testSetup(
changedFiles: await applyReleasePlan(
releasePlan,
await getPackages(tempDir),
config
config,
snapshotConfig?.snapshot
),
tempDir
};
Expand Down Expand Up @@ -653,6 +659,51 @@ describe("apply release plan", () => {
version: "1.0.0"
});
});
it("should use exact versioning when snapshot release is applied, and ignore any range modifiers", async () => {
const releasePlan = new FakeReleasePlan(
[
{
id: "some-id",
releases: [{ name: "pkg-b", type: "minor" }],
summary: "a very useful summary"
}
],
[
{
changesets: ["some-id"],
name: "pkg-b",
newVersion: "1.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

While working on #1008 I've noticed that this test case doesn't reflect how the full thing works in practice. Usually the newVersion should already have the snapshot modifier here. This would be computed here when assembling the release plan:

newVersion: snapshotSuffix
? getSnapshotVersion(
incompleteRelease,
preInfo,
refinedConfig.snapshot.useCalculatedVersion,
snapshotSuffix
)
: getNewVersion(incompleteRelease, preInfo),

This test case still tests that the range modifier gets dropped when snapshot is provided:

let newNewRange = snapshot
? version
: `${getVersionRangeType(depCurrentVersion)}${version}`;

However... this should, sort of, be an impossible setup. Should we validate this anyhow? Should the test case be tweaked to reflect the real usage better?

oldVersion: "1.0.0",
type: "minor"
}
]
);
let { changedFiles } = await testSetup(
"simple-project-caret-dep",
releasePlan.getReleasePlan(),
releasePlan.config,
undefined,
{
snapshot: "canary",
useCalculatedVersionForSnapshots: false
}
);

let pkgPath = changedFiles.find(a =>
a.endsWith(`pkg-a${path.sep}package.json`)
);

if (!pkgPath) throw new Error(`could not find an updated package json`);
let pkgJSON = await fs.readJSON(pkgPath);

expect(pkgJSON).toMatchObject({
name: "pkg-a",
version: "1.1.0",
dependencies: {
"pkg-b": "1.1.0"
}
});
});

describe("internal dependency bumping", () => {
describe("updateInternalDependencies set to patch", () => {
Expand Down
21 changes: 13 additions & 8 deletions packages/apply-release-plan/src/index.ts
Expand Up @@ -111,14 +111,19 @@ export default async function applyReleasePlan(

// iterate over releases updating packages
let finalisedRelease = releaseWithChangelogs.map(release => {
return versionPackage(release, versionsToUpdate, {
updateInternalDependencies: config.updateInternalDependencies,
onlyUpdatePeerDependentsWhenOutOfRange:
config.___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH
.onlyUpdatePeerDependentsWhenOutOfRange,
bumpVersionsWithWorkspaceProtocolOnly:
config.bumpVersionsWithWorkspaceProtocolOnly
});
return versionPackage(
release,
versionsToUpdate,
{
updateInternalDependencies: config.updateInternalDependencies,
onlyUpdatePeerDependentsWhenOutOfRange:
config.___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH
.onlyUpdatePeerDependentsWhenOutOfRange,
bumpVersionsWithWorkspaceProtocolOnly:
config.bumpVersionsWithWorkspaceProtocolOnly
},
snapshot
);
});

let prettierConfig = await prettier.resolveConfig(cwd);
Expand Down
8 changes: 5 additions & 3 deletions packages/apply-release-plan/src/version-package.ts
Expand Up @@ -29,7 +29,8 @@ export default function versionPackage(
updateInternalDependencies: "patch" | "minor";
onlyUpdatePeerDependentsWhenOutOfRange: boolean;
bumpVersionsWithWorkspaceProtocolOnly?: boolean;
}
},
snapshot?: string | boolean | undefined
dotansimha marked this conversation as resolved.
Show resolved Hide resolved
) {
let { newVersion, packageJson } = release;

Expand Down Expand Up @@ -91,8 +92,9 @@ export default function versionPackage(
// leaving those as is would leave the package in a non-installable state (wrong dep versions would get installed)
semver.prerelease(version) !== null
) {
let rangeType = getVersionRangeType(depCurrentVersion);
let newNewRange = `${rangeType}${version}`;
let newNewRange = snapshot
? version
: `${getVersionRangeType(depCurrentVersion)}${version}`;
dotansimha marked this conversation as resolved.
Show resolved Hide resolved
if (usesWorkspaceRange) newNewRange = `workspace:${newNewRange}`;
deps[name] = newNewRange;
}
Expand Down