Fix a bug with prereleases where a package would not be bumped with the highest bump type of all changesets from previous prereleases within the same prerelease run #263

emmatown · 2020-01-26T05:24:40Z

Closes #244

…he highest bump type of all changesets from previous prereleases within the same prerelease run

changeset-bot · 2020-01-26T05:24:42Z

🦋 Changeset is good to go

Latest commit: 28da376

We got this.

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

codecov · 2020-01-26T05:25:03Z

Codecov Report

Merging #263 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #263      +/-   ##
==========================================
+ Coverage   80.04%   80.06%   +0.01%     
==========================================
  Files          45       45              
  Lines        1203     1204       +1     
  Branches      277      278       +1     
==========================================
+ Hits          963      964       +1     
  Misses        229      229              
  Partials       11       11

Impacted Files	Coverage Δ
packages/get-version-range-type/src/index.ts	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b75cb0c...28da376. Read the comment docs.

Andarist · 2020-01-26T09:39:25Z

yarn.lock

@@ -2834,6 +2834,15 @@ get-value@^2.0.3, get-value@^2.0.6:
  resolved "https://registry.yarnpkg.com/get-value/-/get-value-2.0.6.tgz#dc15ca1c672387ca76bd37ac0a395ba2042a2c28"
  integrity sha1-3BXKHGcjh8p2vTesCjlbogQqLCg=

+get-workspaces@^0.5.0:


seems like this shouldn't land in yarn.lock?

It's probably because of Manypkg, it's pretty much fine.

interesting - I would expect no change if @manypkg/cli relies on get-workspaces (which it does), seems like on master there is no get-workspaces entry in yarn.lock, which seems incorrect? unless master yarn.lock got corrupted somehow?

I would guess that this is the first time that the yarn.lock was updated since the last release so that's why there's no get-workspaces entry in the yarn.lock in master.

Even with that - yarn should always use "external" version if a dependency relies on it, shouldnt it? So this entry should be there all along, it's not safe to reuse workspace version in such situation.

It ain't much of a problem, this just has surprised me and I wonder why this has happened out loud 😅

Andarist · 2020-01-26T09:47:59Z

packages/assemble-release-plan/src/index.ts

@@ -62,6 +64,36 @@ function assembleReleasePlan(
  // flattened down to one release per package, having a reference back to their
  // changesets, and with a calculated new versions
  let releases = flattenReleases(changesets, workspaces);
+
+  // for every release in pre mode, we want versions to be bumped to the highest bump type


I'm not sure if I understand the problem correctly, but from what I understand this was caused by the fact that new changesets (the one not added to pre.json yet) could have only lower release types than what was already released before (so what was already added to pre.json before).

If that's the case then I'm wondering - why this had not affected Emotion? We had to have such situation there, I've often~ released only patches when in pre mode.

Also - shouldn't really package.json#version be accounted for as well, rather than just changesets? It's of course not advised to handle stuff manually once you start using changesets, but this could happen and this logic here could lead to unexpected results?

You're understanding the problem correctly.

If that's the case then I'm wondering - why this had not affected Emotion? We had to have such situation there, I've often~ released only patches when in pre mode.

Good point, I just looked into it a bit and it looks like it's because of linked and I have a hunch that it'll cause a bug so I'll look into it more soon.

Also - shouldn't really package.json#version be accounted for as well, rather than just changesets? It's of course not advised to handle stuff manually once you start using changesets, but this could happen and this logic here could lead to unexpected results?

Yeah, that could be a possibility for confusion but what would accounting for package.json#version mean? For example, if you had a package with 1.0.0 and someone bumped the major to 2.0.0-next.0 manually and we have a minor changeset to apply, what should the new version be? What if they bumped it to 3.0.0-next.0 instead? How do we know when to bump and when we've already bumped to a level? I think the right answer here is what we're doing where we store the version before the prereleases and apply the highest bump type to that.

yeah, I guess there is really no good answer to this because we can think of weird cases for both approaches so we really shouldn't be worried much about manual changes to versions as this just has to be strongly discouraged

one additional thing that comes to my mind - technically with the given approach it's possible to "revert" the scope of changes (by publishing a major prerelease and removing that changeset later), which seems reasonable - but also somewhat dangerous for future releases? i think it really isnt much of a real-life scenario so I wouldnt be bothered with it

one additional thing that comes to my mind - technically with the given approach it's possible to "revert" the scope of changes (by publishing a major prerelease and removing that changeset later), which seems reasonable - but also somewhat dangerous for future releases? i think it really isnt much of a real-life scenario so I wouldnt be bothered with it

Yeah, that's true. I agree that it's not a concern.

If that's the case then I'm wondering - why this had not affected Emotion? We had to have such situation there, I've often~ released only patches when in pre mode.

I actually was wondering this myself when I hit the issue.

Also - shouldn't really package.json#version be accounted for as well, rather than just changesets? It's of course not advised to handle stuff manually once you start using changesets, but this could happen and this logic here could lead to unexpected results?

From an explaining how version calculation happens, not taking package.json into account is much easier to explain. Pre-release semver is calculated using both released and unreleased changesets rather than just released changesets

I think this comment could be simplified.

When calculating pre-release versions, the highest version bump of released changesets also need to be applied to the initial pre-release version otherwise calculated version number can be lower than the last released version of the package.

Andarist · 2020-01-26T09:50:01Z

.changeset/dirty-coins-enjoy.md

+"@changesets/cli": patch
+---
+
+Fix a bug with prereleases where a package would not be bumped with the highest bump type of all changesets from previous prereleases within the same prerelease run


This message is a little bit confusing to me, I had to really focus to understand this and I'm still unsure if I got this correctly. Especially the last part is confusing for me (in relation to what is being written before) - "within the same prerelease run" is vague and hard to tell what does it mean in this context.

Maybe this instead?

Fixed a bug in version calculation where only the unreleased pre-release changesets were taken into account when calculating the semver, not previously released changesets.

JakeGinnivan

LGTM, thanks for fixing this issue. Dropped a few comments on as well

JakeGinnivan · 2020-01-28T01:37:28Z

packages/cli/src/commands/version/version.test.ts

@@ -586,4 +586,52 @@ describe("pre", () => {
      }
    ]);
  });
+  it("should use the highest bump type for between all prereleases for every prerelease", async () => {


Nice tests, really easy to read 👍

JakeGinnivan · 2020-01-28T01:39:31Z

packages/assemble-release-plan/src/index.ts

@@ -62,6 +64,36 @@ function assembleReleasePlan(
  // flattened down to one release per package, having a reference back to their
  // changesets, and with a calculated new versions
  let releases = flattenReleases(changesets, workspaces);
+
+  // for every release in pre mode, we want versions to be bumped to the highest bump type


If that's the case then I'm wondering - why this had not affected Emotion? We had to have such situation there, I've often~ released only patches when in pre mode.

I actually was wondering this myself when I hit the issue.

Also - shouldn't really package.json#version be accounted for as well, rather than just changesets? It's of course not advised to handle stuff manually once you start using changesets, but this could happen and this logic here could lead to unexpected results?

From an explaining how version calculation happens, not taking package.json into account is much easier to explain. Pre-release semver is calculated using both released and unreleased changesets rather than just released changesets

JakeGinnivan · 2020-01-28T01:48:10Z

packages/assemble-release-plan/src/index.ts

@@ -62,6 +64,36 @@ function assembleReleasePlan(
  // flattened down to one release per package, having a reference back to their
  // changesets, and with a calculated new versions
  let releases = flattenReleases(changesets, workspaces);
+
+  // for every release in pre mode, we want versions to be bumped to the highest bump type


I think this comment could be simplified.

When calculating pre-release versions, the highest version bump of released changesets also need to be applied to the initial pre-release version otherwise calculated version number can be lower than the last released version of the package.

JakeGinnivan · 2020-01-28T01:50:25Z

packages/assemble-release-plan/src/index.ts

+  // we don't want to add any new releases, we only want to update ones that will already happen
+  // because if they're not being released, the version will already have been bumped with the highest bump type
+  if (updatedPreState !== undefined && updatedPreState.mode !== "exit") {
+    let releasesFromUnfilteredChangesets = flattenReleases(


Does this function return released AND unreleased changesets?

If it only contains released then the opposite issue could occur when the released were only a minor bump but the major contains a major?

It contains released and unreleased.

emmatown · 2020-01-31T00:45:17Z

I "fixed" the "bug" that meant that this bug didn't affect Emotion. I say those things in quotation marks because the bugs were mostly covered up by the slightly confusing nature of semver.inc though I trust the changes in 65fec0e more than relying on how semver.inc deals with pre versions even though I haven't been able to come up with a test case for it.

Fix a bug with prereleases where a package would not be bumped with t…

36cd63b

…he highest bump type of all changesets from previous prereleases within the same prerelease run

A better test name

85ff672

emmatown requested a review from Noviny January 26, 2020 05:25

Andarist reviewed Jan 26, 2020

View reviewed changes

JakeGinnivan approved these changes Jan 28, 2020

View reviewed changes

emmatown added 4 commits January 31, 2020 11:12

Fix some things and add a VSCode config for debugging tests

65fec0e

Remove a console.log

b75cb0c

Merge branch 'master' into fix-pre-bug

8fb9ab1

Update dirty-coins-enjoy.md

28da376

emmatown requested review from Noviny and removed request for Noviny January 31, 2020 00:46

Noviny approved these changes Jan 31, 2020

View reviewed changes

Noviny merged commit 1282ef6 into master Jan 31, 2020

Noviny deleted the fix-pre-bug branch January 31, 2020 05:00

github-actions bot mentioned this pull request Jan 31, 2020

Version Packages #269

Merged

Andarist mentioned this pull request Sep 11, 2020

Remove packagesByNameWithVersionsFromBeforePreModeIfInPreMode and releasesFromUnfilteredChangesets #454

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a bug with prereleases where a package would not be bumped with the highest bump type of all changesets from previous prereleases within the same prerelease run #263

Fix a bug with prereleases where a package would not be bumped with the highest bump type of all changesets from previous prereleases within the same prerelease run #263

emmatown commented Jan 26, 2020

changeset-bot bot commented Jan 26, 2020 •

edited

codecov bot commented Jan 26, 2020 •

edited

Andarist Jan 26, 2020

emmatown Jan 26, 2020

Andarist Jan 26, 2020

emmatown Jan 26, 2020

Andarist Jan 26, 2020

Andarist Jan 26, 2020

emmatown Jan 26, 2020

Andarist Jan 26, 2020

emmatown Jan 26, 2020

JakeGinnivan Jan 28, 2020

JakeGinnivan Jan 28, 2020

Andarist Jan 26, 2020

JakeGinnivan Jan 28, 2020

JakeGinnivan left a comment •

edited

JakeGinnivan Jan 28, 2020

JakeGinnivan Jan 28, 2020

JakeGinnivan Jan 28, 2020

JakeGinnivan Jan 28, 2020

emmatown Jan 31, 2020

emmatown commented Jan 31, 2020

Fix a bug with prereleases where a package would not be bumped with the highest bump type of all changesets from previous prereleases within the same prerelease run #263

Fix a bug with prereleases where a package would not be bumped with the highest bump type of all changesets from previous prereleases within the same prerelease run #263

Conversation

emmatown commented Jan 26, 2020

changeset-bot bot commented Jan 26, 2020 • edited

🦋 Changeset is good to go

codecov bot commented Jan 26, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakeGinnivan left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmatown commented Jan 31, 2020

changeset-bot bot commented Jan 26, 2020 •

edited

codecov bot commented Jan 26, 2020 •

edited

JakeGinnivan left a comment •

edited