Skip to content

fix: correctly specify return type of CANCEL_SYMBOL - #601

Open
gameroman wants to merge 2 commits into
bombshell-dev:mainfrom
gameroman:fix-types
Open

fix: correctly specify return type of CANCEL_SYMBOL#601
gameroman wants to merge 2 commits into
bombshell-dev:mainfrom
gameroman:fix-types

Conversation

@gameroman

@gameroman gameroman commented Sep 8, 2026

Copy link
Copy Markdown
Member

What does this PR do?

Closes #600

Type of change

  • Bug fix
  • Feature
  • Refactor (no behavior change)
  • Documentation
  • Performance improvement
  • Tests
  • Chore (dependencies, CI, tooling)

Checklist

  • pnpm test passes (or targeted tests for my change)
  • pnpm format has been run
  • I have added/updated tests for my changes (if applicable)
  • I have added a changeset

AI-generated code disclosure

  • This PR includes AI-generated code

@changeset-bot

changeset-bot Bot commented Sep 8, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 8ae1524

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

This PR includes changesets to release 2 packages
Name Type
@clack/core Patch
@clack/prompts 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

@pkg-pr-new

pkg-pr-new Bot commented Sep 8, 2026

Copy link
Copy Markdown

commit: 8ae1524

@gameroman
gameroman requested review from 43081j and dreyfus92 and removed request for 43081j September 8, 2026 13:53
@trueberryless

Copy link
Copy Markdown
Contributor

Could you add a changeset with a patch bump, pls?

@dreyfus92 dreyfus92 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this looks good! but, could we add a type-level regression test? something like

if (isCancel(value)) return;
expectTypeOf(value).toEqualTypeOf<string>();

inside an existing text or path test. tsc passed on the 1.8.0 base, so nothing today would fail if the narrowing drifts again. expectTypeOf is already available from vitest and the test dirs are typechecked in CI.

cc @43081j

@43081j

43081j commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

we do already type check via tsc if i remember correctly. so i think instead of having vitest redo this, just write a regular unit test that uses isCancel narrowing and the compiler will catch it instead of vitest.

otherwise vitest has to run typescript again internally

e.g.

if (isCancel(result)) {
  expect(result).toBe(CANCEL_SYMBOL);
} else {
  expect.fail();
}

something like this

@dreyfus92

Copy link
Copy Markdown
Member

so we're aligned on this one. @gameroman please add a regular unit test to verify there isn't a regression with this one 👀

@gameroman

Copy link
Copy Markdown
Member Author

we do already type check via tsc if i remember correctly. so i think instead of having vitest redo this, just write a regular unit test that uses isCancel narrowing and the compiler will catch it instead of vitest.

What Paul suggested will also only checked once when we run by tsc, vitest will ignore it if you don't do vitest --typecheck

expectTypeOf(value).toEqualTypeOf<string>()

@43081j

43081j commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Typescript won't check those assertions, vitest does in type check mode. So it's dead code in a regular type check.

Vitest type assertions are only useful if you don't already run tsc yourself

@gameroman

Copy link
Copy Markdown
Member Author

Typescript won't check those assertions

It does

https://github.com/gameroman/clack/actions/runs/34639636806/job/103395892421?pr=1

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.

[Bug] isCancel no longer narrows prompt results after 1.5.0

4 participants