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

SetParameterType: compatible with break change about in typescript 5.4 #835

Merged
merged 4 commits into from Mar 18, 2024

Conversation

Emiyaaaaa
Copy link
Collaborator

@Emiyaaaaa Emiyaaaaa commented Mar 18, 2024

  1. Compatible with break change about the rest index in array in typescript 5.4

2. Replace npm test with npx tsc in github workflow to make testing more comprehensive

related #831

@Emiyaaaaa Emiyaaaaa changed the title SetParameter: compatible with break change about in typescript 5.4 SetParameterType: compatible with break change about in typescript 5.4 Mar 18, 2024
@@ -37,4 +37,4 @@ jobs:
node-version: 18
- run: npm install
- run: npm install typescript@${{ matrix.typescript-version }}
- run: npx tsc
- run: npm test
Copy link
Collaborator

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

Copy link
Collaborator Author

@Emiyaaaaa Emiyaaaaa Mar 18, 2024

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?

Copy link
Collaborator

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.

Copy link
Collaborator

@voxpelli voxpelli Mar 18, 2024

Choose a reason for hiding this comment

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

See

import {expectTypeOf} from 'expect-type';
and
import {expectTypeOf} from 'expect-type';

Though I'm a bit uncertain if those are actually run using tsc rather than tsd

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it

Copy link
Collaborator Author

@Emiyaaaaa Emiyaaaaa Mar 18, 2024

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 than tsd

Unfortunately it's using tsd.

Directory test-d/ was excluded in tsconfig. https://github.com/sindresorhus/type-fest/blob/4f14bff7321b9a9876dca0719945423abd6b4da1/tsconfig.json#L14C2-L14C16

@sindresorhus sindresorhus merged commit 2bc451e into sindresorhus:main Mar 18, 2024
6 checks passed
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.

None yet

3 participants