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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 8 commits into from Jul 7, 2022
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2b10211
Dont't include ignored packages when adding a changeset
Feb 4, 2022
4791c19
Add test
Feb 7, 2022
4c5ecc8
Merge branch 'main' into ignored-packages
mskelton Feb 12, 2022
7f87916
Merge branch 'main' into ignored-packages
mskelton Mar 18, 2022
0dd1c10
Merge branch 'main' into ignored-packages
mskelton Mar 28, 2022
4da1ec8
Merge branch 'main' into ignored-packages
mskelton Apr 9, 2022
1a81331
Merge branch 'main' into ignored-packages
mskelton May 30, 2022
9dc2545
Merge branch 'main' into ignored-packages
emmatown Jul 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/young-queens-add.md
View file
Open in desktop
@@ -0,0 +1,5 @@
---
"@changesets/cli": patch
---

Ignored packages are no longer listed when adding a changeset.
14 changes: 14 additions & 0 deletions packages/cli/src/commands/add/__tests__/add.ts
View file
Open in desktop
Expand Up @@ -204,4 +204,18 @@ describe("Changesets", () => {
})
);
});
it("should not include ignored packages in the prompt", async () => {
const cwd = await f.copy("internal-dependencies");

mockUserResponses({ releases: { "pkg-a": "patch" } });
await addChangeset(
cwd,
{ empty: false },
{ ...defaultConfig, ignore: ["pkg-b"] }
);

// @ts-ignore
const { choices } = askCheckboxPlus.mock.calls[0][1][0];
expect(choices).toEqual(["pkg-a", "pkg-c"]);
});
});
13 changes: 8 additions & 5 deletions packages/cli/src/commands/add/index.ts
View file
Open in desktop
Expand Up @@ -23,7 +23,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

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

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

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.

pkg => !config.ignore.includes(pkg.packageJson.name)
);
const changesetBase = path.resolve(cwd, ".changeset");

let newChangeset: UnwrapPromise<ReturnType<typeof createChangeset>>;
Expand All @@ -39,10 +41,11 @@ export default async function add(
ref: config.baseBranch
});
const changePackagesName = changedPackages
.filter(a => a)
.map(pkg => pkg.packageJson.name);
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 •
edited

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.


newChangeset = await createChangeset(changePackagesName, packages);
printConfirmationMessage(newChangeset, packages.length > 1);

if (!newChangeset.confirmed) {
newChangeset = {
Expand Down