-
-
Notifications
You must be signed in to change notification settings - Fork 542
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
SetParameterType
: compatible with break change about in typescript 5.4
#835
SetParameterType
: compatible with break change about in typescript 5.4
#835
Conversation
SetParameter
: compatible with break change about in typescript 5.4SetParameterType
: compatible with break change about in typescript 5.4
.github/workflows/main.yml
Outdated
@@ -37,4 +37,4 @@ jobs: | |||
node-version: 18 | |||
- run: npm install | |||
- run: npm install typescript@${{ matrix.typescript-version }} | |||
- run: npx tsc | |||
- run: npm test |
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 makes it test more than just typescript and may fail for unrelated reasons
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.
How about using npm test
in ts-canary.yml?
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.
As was mentioned in the issue, tsd
ships with its own copy of typescript and as such isn't affected by any changes to the typescript
dependency: #831 (comment)
One would instead have to do what I believe @shicks and @trevorade are doing: Use eg. expect-type
instead: #499
And since expect-type
is validated as part of running tsc
itself, no changes to the GitHub Actions would be done then either.
That said: Replacing tsd
with expect-type
is a much broader topic, as highlighted by eg #499
Though: Since expect-type
is already a dev dependency of type-fest
as far as I remember it could be good to add a test using that and ensure that breaks the canary tests correctly, at least that way we won't see a regression again.
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.
See
Line 1 in 4f14bff
import {expectTypeOf} from 'expect-type'; |
type-fest/test-d/required-deep.ts
Line 2 in 4f14bff
import {expectTypeOf} from 'expect-type'; |
Though I'm a bit uncertain if those are actually run using tsc
rather than tsd
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.
Got it
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.
Though I'm a bit uncertain if those are actually run using
tsc
rather thantsd
Unfortunately it's using tsd
.
Directory test-d/
was excluded in tsconfig. https://github.com/sindresorhus/type-fest/blob/4f14bff7321b9a9876dca0719945423abd6b4da1/tsconfig.json#L14C2-L14C16
2. Replacenpm test
withnpx tsc
in github workflow to make testing more comprehensiverelated #831