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

fix: handle pnpm monorepo workspace:^ version correctly (#111) #128

Merged
merged 4 commits into from
Sep 14, 2022
Merged

fix: handle pnpm monorepo workspace:^ version correctly (#111) #128

merged 4 commits into from
Sep 14, 2022

Conversation

lyh543
Copy link
Contributor

@lyh543 lyh543 commented Sep 8, 2022

resolve #111: support of replacing pnpm's workspace: dependencies version. pnpm docs

I modify current existing tests, since I think it should work like this :P. Anyway, I may have misunderstanding on this project, so I'm happy to discuss with contributor.

After I'm allowed to modify current existing tests, I will add integration tests on monorepo that uses pnpm workspace: protocol.


Update: integration tests added.

@lyh543 lyh543 marked this pull request as draft September 8, 2022 14:23
@lyh543 lyh543 changed the title replace pnpm workspace:^ correctly (#111) fix: handle pnpm monorepo workspace:^ version correctly (#111) Sep 8, 2022
@lyh543
Copy link
Contributor Author

lyh543 commented Sep 8, 2022

Why I modify the existing tests

Due to the --deps.bump docs from README:

Define deps version update rule.
override — replace any prev version with the next one,
satisfy — check the next pkg version against its current references. If it matches (* matches to any, 1.1.0 matches 1.1.x, 1.5.0 matches to ^1.0.0 and so on) release will not be triggered, if not override strategy will be applied instead;
inherit will try to follow the current declaration version/range. ~1.0.0 + minor turns into ~1.1.0, 1.x + major gives 2.x, but 1.x + minor gives 1.x so there will be no release, etc.

In yarnWorkspacesPackagesCarret, all deps are defined like ^0.0.0. In my opinion, with inherit as rule, deps version in new package.json should keep the ^1.0.0 style, so I update the test expectation.

Discussions are welcome👋

@antongolub
Copy link
Collaborator

We also experimented with the workspace protocol in https://github.com/semrel-extra/zx-bulk-release/blob/master/src/main/js/deps.js#L56. I'm not sure if this implementation is correct, but it might be a starting point.

@lyh543
Copy link
Contributor Author

lyh543 commented Sep 8, 2022

We also experimented with the workspace protocol in https://github.com/semrel-extra/zx-bulk-release/blob/master/src/main/js/deps.js#L56. I'm not sure if this implementation is correct, but it might be a starting point.

This implementation with regex is really simple! I'm so amazed that I refactored my code immediately.

BTW, can you review the part that modified existing tests? I wonder if I caught an bug or just misunderstood the code.

@lyh543 lyh543 marked this pull request as ready for review September 9, 2022 06:57
@antongolub
Copy link
Collaborator

antongolub commented Sep 9, 2022

@Badisi, @moroine, could you pls take a look?

Copy link
Contributor

@moroine moroine left a comment

Choose a reason for hiding this comment

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

About the inherit change I think it's a breaking change but we didn't found out why we got this behavior. You can have a look at the discussion here.

#101 (comment)

cc: @antongolub

@@ -306,7 +328,7 @@ const updateManifestDeps = (pkg, writeOut = true, bumpStrategy = "override", pre
scopes.forEach((scope) => {
if (scope[dependency.name]) {
scope[dependency.name] = resolveNextVersion(
get(dependency, "_lastRelease.version", "0.0.0"),
scope[dependency.name],
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the bugfix of inherit 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like that, so there's no need for yet another br change.

@antongolub antongolub merged commit 2d083e0 into dhoulb:master Sep 14, 2022
@antongolub
Copy link
Collaborator

🎉 This PR is included in version 3.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: replace pnpm workspace:^ correctly
3 participants