fix: Invalid comparator semver error when using tag versions #1047

patzick · 2022-12-22T22:17:48Z

Hey, great job with the project!

Small fix to cover the case when you're using tag versions.

Let's say you publish snapshots on npm with our bulbosaur example from docs :)
You have examples in monorepo which can be fired from web container like stackblitz or codesandbox, but we don't want (and should not) commit snapshot releases with versions back to repo. Tags are solutions to that problem, yet they're not a valid semver ranges, so we have for example (my case is canary tag):

which breaks whole release. I'll be very grateful for quickly releasing this fix if all looks good for you here. Cheers!

changeset-bot · 2022-12-22T22:17:51Z

🦋 Changeset detected

Latest commit: bdec3d0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@changesets/cli	Patch
@changesets/apply-release-plan	Patch

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

codesandbox-ci · 2022-12-22T22:18:16Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

patzick · 2022-12-30T10:29:40Z

hey @Andarist!
Would you mind taking a look at this PR? It'd be super helpful to have this fix released :)

Andarist · 2022-12-30T11:27:23Z

Hmm, the goal of the change looks OK to me. if you are using a tag - that's your choice and Changesets should leave this alone. However, I wonder if your use case is correct - snapshot releases were not designed to be committed. Whatever changes they make to package.json/CHANGELOG.md/etc files should be treated as disposable.

Andarist · 2022-12-30T11:32:27Z

packages/apply-release-plan/src/version-package.ts

@@ -84,14 +84,15 @@ export default function versionPackage(
          depCurrentVersion = workspaceDepVersion;
        }
        if (
+          semver.validRange(depCurrentVersion) !== null &&


tags should already be allowed (and ignored), please take a look at the logic here:

changesets/packages/get-dependents-graph/src/get-dependency-graph.ts

Lines 114 to 118 in 87e9bb8

// `depRange` could have been a tag and if a tag has been used there might have been a reason for that

// we should not count this as a local monorepro dependant

if (!range) {

continue;

}

So if this isn't working for you... I wonder what went wrong in between. Why do we end up with a release for a package here if it's not part of the dep graph? Or is the package in question (example package) versioned on its own as well - so it wasn't pulled into the whole release by its dependencies but it's rather released on its own? If that's the case - why is it released/versioned? Could it be avoided?

probably because the package itself is correct and we're upgrading this package version; but then it detects other dependency packages which are in monorepo and tries to upgrade this dependencies as well, which are marked as tags - and this is where the problem is. If you'd revert change that test will not pass with exactly the error I'm having while changeset version is invoked

So if we take vue-demo-store as an example of this issue - do you explicitly create a changeset file for this vue-demo-store package? If yes - what for?

so you have

/packages /package-a --> 0.1.0 /templates /some-template --> 0.2.3 dependency to package-a -> not workspace:* but TAG_NAME

so when you add changeset to some-template we should have it as for example 0.2.4 and nothing else. andwhen you update package-a then dependency at some-template should not change even if some-template has a correct range version

So if we take vue-demo-store as an example of this issue - do you explicitly create a changeset file for this vue-demo-store package? If yes - what for?

yes, we do create changeset for that, reasoning:

vue-demo-store is a template, it's not published on npm as it's not upgradable, but also templates have their issues, so thanks to that we want to keep changelog for templates which features/fixes were added and when (versioning them). It helps to keep cleaner history of changes and inform our users what has changed since last time they used this template

Could you add a version-level test for this that would depict the whole flow more closely? You can add it here. Th test case that you already added shouldn't be removed.

sure! I'm on it

patzick · 2022-12-30T11:38:19Z

@Andarist thank you for your response. Maybe I wasn't clear but definitely we're not committing snapshots, but on the other hand we have stackblitz templates which should work with current master branch (so snapshot) published on npm, that's why templates are using snapshot tag, but on local envs it resolves to packages from monorepo.

Quick real life case:
https://github.com/shopware/frontends/blob/65d6647c5b3ca94733c4660dfa54bfec6292afa4/templates/vue-demo-store/package.json#L15 template package file, which on stackblitz takes tag version from npm and locally from package.

But having this setup it crasches during changelog version and that's why I creates this PR :)

Andarist · 2022-12-30T11:42:26Z

Could you take a look at my other comment? I feel like I'm missing something about the nature of this bug (or your setup) - and I would like to commit this change only after understanding the full picture.

patzick · 2022-12-30T12:07:38Z

Great, I appreciate you taking the time to dive deeper into that to understand my issue. I'm aware this setup is not a common one, but I have good reasons for that, which I hope are also making sense to you :)

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

patzick · 2023-01-04T13:12:35Z

hey @Andarist, can I ask you to re-review this one? :)

patzick · 2023-01-09T08:07:34Z

@Andarist really sorry to ping you like this all the time :(
unfortunately without this fix we're blocked with proper releasing, as the process just crashes

patzick · 2023-01-17T09:59:36Z

hello @Andarist, you're probably busy, it there anyone else I could ping to make this go forward?

patzick · 2023-06-26T09:59:53Z

Hey @Andarist, as a patch it's working for 5 months now. Is there anything blocking you from merging that? Just to refresh memory - it's just making sure the whole process is not failing when we have an npm tag version instead of semver.

Andarist · 2023-06-27T08:28:27Z

@patzick could you resolve conflicts here? Sorry for not getting back to you earlier here

patzick · 2023-06-27T09:47:16Z

@Andarist, no problem, thanks for getting back at this!

The conflict was caused by this: https://github.com/changesets/changesets/pull/1176/files#diff-73bc94618d4ce1bc263e6ecb9e90dee49072a5d789137c6b530dccd1f99fbcabL91-R95
all resolved now and my import also changed in the same tree-shake convention. Test passing locally

* chore: bump dependencies * chore: revert i18n plugin update * chore: new patch after update, related changesets/changesets#1047 * chore: add changeset

patzick · 2023-07-03T07:41:06Z

Hey @Andarist, can you merge it? After the new release, I needed to patch it once again in our code and I see new merge movements so probably new release is coming soon :)

* chore: bump dependencies * chore: revert i18n plugin update * chore: new patch after update, related changesets/changesets#1047 * chore: add changeset

Andarist · 2023-07-05T07:37:12Z

LGTM, it only needs some minor code shuffling related to the added test case.

Andarist · 2023-07-12T09:41:27Z

packages/apply-release-plan/src/index.test.ts

@@ -1356,6 +1356,111 @@ describe("apply release plan", () => {
            },
          });
        });
+        it("should not update internal version for tags", async () => {


I wonder, is this case right now? Shouldn't it be just using a pkg-a: patch changeset or smth like that to test that pkg-b doesn't get updated transitively? Perhaps I misunderstand the intention of this test though

The purpose of this test is that dependencies are not changed (not bumped) as they're using the tag version, not a numeric version. So even if both packages are updated, they should not change anything inside dependencies as tags should be static, not semver specific :)

patzick · 2023-11-07T14:15:42Z

Woah, I somehow missed that there was another question here. As I thought LGTM covered all changes and it's merged already :)
@Andarist do you think we can merge it so I won't have to patch it again when new version will be released? :)

patzick · 2023-11-29T08:25:08Z

Hey @Andarist - is there anything else keeping you from merging this PR?
Unfortunately, with every new release, I need to patch it with that fix to avoid crashes in the process :(

Let me know if I can assist somehow more to go through with that!

…rged

patzick · 2024-02-23T12:50:12Z

hey @Andarist!
I'm coming back with the question if we could merge this fix, it has been tested (as patches) in our project for over a year and the only thing is doing is preventing crashes by additional checks; nothing fancy, very useful though :)

codecov · 2024-05-29T08:47:17Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.02%. Comparing base (b036aae) to head (bdec3d0).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1047      +/-   ##
==========================================
+ Coverage   80.99%   81.02%   +0.02%     
==========================================
  Files          54       54              
  Lines        2210     2213       +3     
  Branches      652      654       +2     
==========================================
+ Hits         1790     1793       +3     
  Misses        416      416              
  Partials        4        4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

patzick · 2024-05-29T08:52:16Z

Hello, @Andarist, I see some movements in the repo.
Could you please go back to this fix and merge it?

Andarist · 2024-06-29T21:39:17Z

packages/apply-release-plan/src/get-changelog-entry.ts

@@ -3,6 +3,7 @@ import { ChangelogFunctions, NewChangesetWithCommit } from "@changesets/types";
 import { ModCompWithPackage } from "@changesets/types";
 import startCase from "lodash.startcase";
 import { shouldUpdateDependencyBasedOnConfig } from "./utils";
+import validRange from "semver/ranges/valid";


We could also try doing smth about invalid tags (as in - continue to throw). To check what a valid tag is we could do:

function isValidTag(tag: string) { return encodeURIComponent(tag) === tag; }

This can be verified here.

But honestly, I don't think it's worth it.

.changeset/famous-poems-jog.md

Andarist · 2024-06-29T22:02:50Z

rmakara · 2024-06-30T19:11:04Z

Respect! 💙

fix: Invalid comparator semver error when using tag versions

87e9bb8

Andarist reviewed Dec 30, 2022

View reviewed changes

test: add testing scenario for versioning with tags

6927a6b

patzick commented Dec 30, 2022

View reviewed changes

packages/cli/src/commands/version/version.test.ts Outdated Show resolved Hide resolved

chore: typo

a7e2100

patzick requested a review from Andarist December 30, 2022 13:05

shopwareBot pushed a commit to shopware/frontends that referenced this pull request Jan 25, 2023

chore: patch changeset until - changesets/changesets#1047

8781dca

patzick added 2 commits March 29, 2023 08:51

Merge branch 'main' into fix/semver-invalid-comparator

a557c34

Merge branch 'main' into fix/semver-invalid-comparator

c7d11f6

Merge branch 'main' into fix/semver-invalid-comparator

3ff327d

patzick added a commit to shopware/frontends that referenced this pull request Jun 29, 2023

chore: new patch after update, related changesets/changesets#1047

6328735

patzick mentioned this pull request Jun 29, 2023

chore: bump dependencies shopware/frontends#295

Merged

patzick added a commit to shopware/frontends that referenced this pull request Jun 29, 2023

chore: bump dependencies (#295)

23a0a53

* chore: bump dependencies * chore: revert i18n plugin update * chore: new patch after update, related changesets/changesets#1047 * chore: add changeset

mkucmus pushed a commit to shopware/frontends that referenced this pull request Jun 30, 2023

chore: bump dependencies (#295)

aed2413

* chore: bump dependencies * chore: revert i18n plugin update * chore: new patch after update, related changesets/changesets#1047 * chore: add changeset

Merge branch 'main' into fix/semver-invalid-comparator

207a532

mdanilowicz pushed a commit to shopware/frontends that referenced this pull request Jul 3, 2023

chore: bump dependencies (#295)

d4e79de

* chore: bump dependencies * chore: revert i18n plugin update * chore: new patch after update, related changesets/changesets#1047 * chore: add changeset

chore: move test case and adjust assertions

ef93d25

patzick requested a review from Andarist July 5, 2023 09:23

Andarist reviewed Jul 12, 2023

View reviewed changes

Merge branch 'main' into fix/semver-invalid-comparator

47b08b8

patzick added a commit to shopware/frontends that referenced this pull request Dec 5, 2023

chore: add patch for changeset until changesets/changesets#1047 is me…

3455b6a

…rged

BrocksiNet pushed a commit to shopware/frontends that referenced this pull request Dec 7, 2023

chore: add patch for changeset until changesets/changesets#1047 is me…

5721343

…rged

Merge branch 'main' into fix/semver-invalid-comparator

73ac6ca

Merge branch 'main' into fix/semver-invalid-comparator

21e649e

Merge branch 'main' into fix/semver-invalid-comparator

b00245b

patzick requested a review from Andarist June 3, 2024 14:33

patzick and others added 3 commits June 27, 2024 10:27

Merge branch 'main' into fix/semver-invalid-comparator

8379f28

make one of the calls lazy

19a69aa

make the other one lazier too

eedc506

Andarist reviewed Jun 29, 2024

View reviewed changes

Andarist added 2 commits June 29, 2024 23:54

tweak tests

d864fe5

tweak test title

7810e2c

Andarist reviewed Jun 29, 2024

View reviewed changes

.changeset/famous-poems-jog.md Outdated Show resolved Hide resolved

Andarist reviewed Jun 29, 2024

View reviewed changes

.changeset/famous-poems-jog.md Show resolved Hide resolved

Apply suggestions from code review

bdec3d0

Andarist merged commit d108fa6 into changesets:main Jun 29, 2024
7 checks passed

github-actions bot mentioned this pull request Jun 29, 2024

Version Packages #1398

Merged

Exkaleburx mentioned this pull request Jul 22, 2024

[Snyk] Upgrade @changesets/cli from 2.27.1 to 2.27.7 Exkaleburx/chakra-ui#113

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Invalid comparator semver error when using tag versions #1047

fix: Invalid comparator semver error when using tag versions #1047

patzick commented Dec 22, 2022

changeset-bot bot commented Dec 22, 2022 •

edited

Loading

codesandbox-ci bot commented Dec 22, 2022 •

edited

Loading

patzick commented Dec 30, 2022

Andarist commented Dec 30, 2022

Andarist Dec 30, 2022

patzick Dec 30, 2022

Andarist Dec 30, 2022

patzick Dec 30, 2022

patzick Dec 30, 2022 •

edited

Loading

Andarist Dec 30, 2022

patzick Dec 30, 2022

patzick commented Dec 30, 2022 •

edited by Andarist

Loading

Andarist commented Dec 30, 2022

patzick commented Dec 30, 2022

patzick commented Jan 4, 2023

patzick commented Jan 9, 2023

patzick commented Jan 17, 2023

patzick commented Jun 26, 2023

Andarist commented Jun 27, 2023

patzick commented Jun 27, 2023

patzick commented Jul 3, 2023

Andarist commented Jul 5, 2023

Andarist Jul 12, 2023

patzick Nov 7, 2023

patzick commented Nov 7, 2023

patzick commented Nov 29, 2023

patzick commented Feb 23, 2024

codecov bot commented May 29, 2024 •

edited

Loading

patzick commented May 29, 2024

Andarist Jun 29, 2024

Andarist commented Jun 29, 2024

rmakara commented Jun 30, 2024

	// `depRange` could have been a tag and if a tag has been used there might have been a reason for that
	// we should not count this as a local monorepro dependant
	if (!range) {
	continue;
	}

fix: Invalid comparator semver error when using tag versions #1047

fix: Invalid comparator semver error when using tag versions #1047

Conversation

patzick commented Dec 22, 2022

changeset-bot bot commented Dec 22, 2022 • edited Loading

🦋 Changeset detected

codesandbox-ci bot commented Dec 22, 2022 • edited Loading

patzick commented Dec 30, 2022

Andarist commented Dec 30, 2022

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

patzick Dec 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patzick commented Dec 30, 2022 • edited by Andarist Loading

Andarist commented Dec 30, 2022

patzick commented Dec 30, 2022

patzick commented Jan 4, 2023

patzick commented Jan 9, 2023

patzick commented Jan 17, 2023

patzick commented Jun 26, 2023

Andarist commented Jun 27, 2023

patzick commented Jun 27, 2023

patzick commented Jul 3, 2023

Andarist commented Jul 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patzick commented Nov 7, 2023

patzick commented Nov 29, 2023

patzick commented Feb 23, 2024

codecov bot commented May 29, 2024 • edited Loading

Codecov Report

patzick commented May 29, 2024

Choose a reason for hiding this comment

Andarist commented Jun 29, 2024

rmakara commented Jun 30, 2024

changeset-bot bot commented Dec 22, 2022 •

edited

Loading

codesandbox-ci bot commented Dec 22, 2022 •

edited

Loading

patzick Dec 30, 2022 •

edited

Loading

patzick commented Dec 30, 2022 •

edited by Andarist

Loading

codecov bot commented May 29, 2024 •

edited

Loading