-
Notifications
You must be signed in to change notification settings - Fork 588
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
Add support for fixed
packages
#690
Conversation
🦋 Changeset detectedLatest commit: 107d706 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
@@ -95,6 +95,44 @@ describe("assemble-release-plan", () => { | |||
expect(releases[1].newVersion).toEqual("1.0.1"); | |||
expect(releases[1].changesets).toEqual([]); | |||
}); | |||
it("should update multiple dependents of a single package", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those two tests previously had been configuring linked
but that wasn't relevant for them so I've moved them to the top block~
@@ -254,189 +292,7 @@ describe("assemble-release-plan", () => { | |||
expect(releases[0].name).toEqual("pkg-a"); | |||
expect(releases[0].newVersion).toEqual("2.0.0"); | |||
}); | |||
it("should assemble release plan for linked packages", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've grouped tests related to the linked packages in a describe block - no significant changes were made to those moved tests
@@ -144,36 +145,6 @@ describe("running version in a simple project", () => { | |||
}); | |||
}); | |||
|
|||
it("should bump packages to the correct versions when packages are linked", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similarly here - I've moved tests related to linked packages to their own block
2efa76c
to
a73cec9
Compare
a73cec9
to
29633c2
Compare
@@ -0,0 +1,87 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This config doesn't actually work, right? Since it's a js file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea - I've just copied the linked-packages
fixtures and modified it slightly. I will verify tomorrow if this can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The option seems fine to me. Def curious to see what the changelog looks like. We might even want changelog generators to have a separate export to define what we say in the case of "this package hasn't changed at all but it's being released so all the versions are the same"
Definitely lots to discuss when it comes to the changelog generation stuff - I will reach out to you in the coming days to talk this through further, but in the meantime you could take a look at this: #683 . I have some new ideas/thoughts on this issue - but the use cases mentioned there are still relevant. |
What is the status of this pull? Would it be possible to try this now if I install changesets from the branch? |
@fivethreeo I'm waiting for review and need to recheck the todo list added in the thread. You can try it out by installing this build:
|
case "minor": | ||
highestReleaseType = "minor"; | ||
break; | ||
// patch never needs to override another value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't patch
need to override a none
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, you are right. none
releases never actually get here - we only with releases that will actually be released here. However, this util is quite generic so better to handle all the cases now than to be sorry later. Going to push a fix for this in a sec.
A test that generates a changelog would be great (I imagine for something being bumped that didn't have any changesets it would just have the version and nothing else?) |
# Conflicts: # packages/apply-release-plan/src/index.test.ts # packages/assemble-release-plan/src/apply-links.ts # packages/assemble-release-plan/src/index.test.ts # packages/cli/src/commands/version/version.test.ts # packages/config/src/index.ts
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. Latest deployment of this branch, based on commit 107d706:
|
… that dont have any changeset associated with them
@mitchellhamilton just added such a test. I think that an "empty" entry is acceptable for now - we should gather feedback from users before introducing more options. It is very likely that most people who want the fixed mode might want aggregate changelogs (#683) and this would "solve" this problem without adding new functions to changelog generators. |
This introduces an alternative to the linked mode. Linked mode is being constantly misunderstood and confuses people while the fixed mode is known from other tools (like Lerna).
TODO:
closes #581
closes #570