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

Dont't include ignored packages when adding a changeset #744

Merged
merged 8 commits into from Jul 7, 2022

Conversation

mskelton
Copy link
Contributor

@mskelton mskelton commented Feb 4, 2022

Fixes #436

Filters ignored packages before displaying the list of packages when adding a new changeset.

@changeset-bot
Copy link

changeset-bot bot commented Feb 4, 2022

🦋 Changeset detected

Latest commit: 9dc2545

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

This PR includes changesets to release 1 package
Name Type
@changesets/cli 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
Copy link

codesandbox-ci bot commented Feb 4, 2022

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:

Sandbox Source
Vanilla Configuration

@mskelton mskelton marked this pull request as ready for review Feb 7, 2022
@@ -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(
Copy link
Member

@Andarist Andarist Feb 11, 2022

Choose a reason for hiding this comment

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

It feels like the filtered result should be used used for createChangeset call but not for the printConfirmationMessage. WDYT?

Copy link
Contributor Author

@mskelton mskelton Feb 12, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member

@mitchellhamilton mitchellhamilton Jul 7, 2022

Choose a reason for hiding this comment

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

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.

@jsheely
Copy link

jsheely commented Mar 17, 2022

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

@mskelton
Copy link
Contributor Author

mskelton commented Apr 9, 2022

@Andarist Any chance of getting this merged in?

@StarpTech
Copy link

StarpTech commented May 26, 2022

Also a ping from me. Please merge it.

@wycats
Copy link

wycats commented Jun 21, 2022

I would also find this extremely useful and consider it a blocker for fully adopting changesets in @starbeamjs/starbeam

@dotansimha
Copy link
Contributor

dotansimha commented Jun 29, 2022

Same here. Anything that can be done to get this merged? :) @Andarist thanks

@silesky
Copy link

silesky commented Jun 30, 2022

@Andarist is there something I can do to help get this across the finish line? Thanks!

@wycats
Copy link

wycats commented Jul 5, 2022

At this point, would it be appropriate to temporarily fork and publish this with the changes?

@mitchellhamilton mitchellhamilton merged commit 84e46d1 into changesets:main Jul 7, 2022
4 checks passed
@github-actions github-actions bot mentioned this pull request Jul 7, 2022
@mskelton mskelton deleted the ignored-packages branch Jul 7, 2022
@wycats
Copy link

wycats commented Jul 7, 2022

Thank you so much!

@wycats
Copy link

wycats commented Jul 12, 2022

At this point, would it be appropriate to temporarily fork and publish this with the changes?

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));
Copy link
Contributor

@glasser glasser Jul 13, 2022

Choose a reason for hiding this comment

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

Is there a missing ! here? See #869.

@wycats
Copy link

wycats commented Jul 13, 2022

Related to #869, I upgraded to 2.23.1 and changeset still seems to pick up stuff in ignores.

If @mskelton is willing to dedicate some time to review, I'd be happy to dive in and try to find a proper fix. @mskelton any interest?

@mskelton
Copy link
Contributor Author

mskelton commented Jul 13, 2022

@wycats #871 was already opened to fix the bug. Somehow I missed the not in my original PR, but there is already a PR to fix it.

@Andarist
Copy link
Member

Andarist commented Jul 13, 2022

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 🙏

@mskelton
Copy link
Contributor Author

mskelton commented Jul 13, 2022

@Andarist I just verified the fix applied in 2.23.2 works correctly. Thanks for the quick resolution to my mistake!

@wycats
Copy link

wycats commented Jul 14, 2022

@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 🙌

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

Successfully merging this pull request may close these issues.

Choosing all packages to bump also includes ignored packages and results in error during version phase
9 participants