-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(types): ensure Chai declaration merge works with TS-Go #8188
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
Conversation
✅ Deploy Preview for vitest-dev ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
interface ContainSubset { | ||
(expected: any): Assertion | ||
} | ||
|
||
interface Assertion { | ||
containSubset: (expected: any) => Assertion | ||
containSubset: ContainSubset | ||
} |
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 change is not critically necessary for supporting TS-Go. However while touching this area of the code I noticed that @types/chai
does this:
interface ContainSubset {
(expected: any): Assertion;
}
interface Assertion extends LanguageChains, NumericComparison, TypeComparison {
...
containSubset: ContainSubset;
...
}
And this change makes the polyfill here more compliant with what @types/chai
is doing. Currently if someone changed ContainSubset
it would break vitest
.
// eslint-disable-next-line ts/method-signature-style | ||
containSubset(val: any, exp: any, msg?: string): void |
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 is the change that's critical for TS-Go. You can test this locally by running these commands in packages/vitest
:
pnpm add -D @typescript/native-preview
pnpm exec tsgo
Before this change you would get these errors:
src/types/global.ts:20:7 - error TS2300: Duplicate identifier 'containSubset'.
20 containSubset: (val: any, exp: any, msg?: string) => void
~~~~~~~~~~~~~
../../node_modules/.pnpm/@[email protected]/node_modules/@types/chai/index.d.ts:538:13 - 'containSubset' was also declared here.
538 containSubset(val: any, exp: any, msg?: string): void;
After, these errors go away.
Could I get a rerun on "Browsers: node-20, macos-latest" and "Build&Test: node-20, windows-latest"?
The windows error is:
I don't see how either of these could be caused by this change and I see these checks failing elsewhere. |
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.
LGTM!
Description
Context: Currently TypeScript accepts code like this:
While it does not accept code like this:
The only difference being the ordering.
By comparison, in TypeScript Go neither of these are accepted. See microsoft/typescript-go#1192 (comment) for official confirmation by Anders Hejlsberg (lead architect of TypeScript). I noticed this effected Vitest due to its merge of
Chai.Assert.containSubset
as a callback instead of as a method, leading to a conflict with@types/chai
.No associated issue because it's a small types-only fix.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
pnpm-lock.yaml
unless you introduce a new test example.Tests
pnpm test:ci
.Changesets
feat:
,fix:
,perf:
,docs:
, orchore:
.