Dont't include ignored packages when adding a changeset #744
Conversation
🦋 Changeset detectedLatest commit: 9dc2545 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
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 9dc2545:
|
| @@ -22,7 +22,9 @@ export default async function add( | |||
| { empty, open }: { empty?: boolean; open?: boolean }, | |||
| config: Config | |||
| ) { | |||
| const packages = await getPackages(cwd); | |||
| const packages = (await getPackages(cwd)).packages.filter( | |||
There was a problem hiding this comment.
It feels like the filtered result should be used used for createChangeset call but not for the printConfirmationMessage. WDYT?
There was a problem hiding this comment.
Hmm, not sure. I see what you are getting at that it could mess up the resulting value of repoHasMultiplePackages, but if a repo has two packages but one is ignored, should we consider the repo as having one or multiple packages? That's where I'm a bit unsure of what you would prefer to happen.
There was a problem hiding this comment.
The message added when repoHasMultiplePackages is true is about bumping dependents which if there is only one non-ignored package is irrelevant so I'd say the behavior in this PR is correct.
|
I'd like to see this get merged in soon. Having to skip over sample apps in our mono repo every time we publish is becoming a chore |
|
@Andarist Any chance of getting this merged in? |
|
Also a ping from me. Please merge it. |
|
I would also find this extremely useful and consider it a blocker for fully adopting changesets in @starbeamjs/starbeam |
|
Same here. Anything that can be done to get this merged? :) @Andarist thanks |
|
@Andarist is there something I can do to help get this across the finish line? Thanks! |
|
At this point, would it be appropriate to temporarily fork and publish this with the changes? |
|
Thank you so much! |
This is one of happiest thumbs-downs I've ever received on Github 😄 |
| newChangeset = await createChangeset(changePackagesName, packages.packages); | ||
| printConfirmationMessage(newChangeset, packages.packages.length > 1); | ||
| .map(pkg => pkg.packageJson.name) | ||
| .filter(pkgName => config.ignore.includes(pkgName)); |
There was a problem hiding this comment.
Is there a missing ! here? See #869.
|
I've released the said fix - could you try it out and report back if it's working now? The fix itself was definitely good and something that has been accidentally overlooked before. If somebody could add tests for this behavior - it would be great 🙏 |
|
@Andarist I just verified the fix applied in 2.23.2 works correctly. Thanks for the quick resolution to my mistake! |
|
@mskelton I apologize if my tone sounds aggressive or annoying. I'm just really excited about the prospect of building changesets into the Starbeam release workflow, so I'm eager (overeager?) to help 😳. I already did a handful of releases using Changesets, and I'm cautiously optimistic that with this bugfix (and a few minor tweaks to how we're using it now, including figuring out how to get the exact automation we need for PRs) that we'll be using it for years to come. Thanks so much for your engagement 🙌 |
Fixes #436
Filters ignored packages before displaying the list of packages when adding a new changeset.