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

TS 5.4.2 breaks ConditionalKeys and SetParameterType #831

Closed
trevorade opened this issue Mar 13, 2024 · 16 comments · Fixed by #852
Closed

TS 5.4.2 breaks ConditionalKeys and SetParameterType #831

trevorade opened this issue Mar 13, 2024 · 16 comments · Fixed by #852

Comments

@trevorade
Copy link
Contributor

trevorade commented Mar 13, 2024

For ConditionalKeys, the root issue is: microsoft/TypeScript#56644 which sound slike it will not be fixed. Basically, the NonNullable call in the ConditionalKeys type is ignored and does not run. Based on that issue, it's possible this type can be rewritten to be compatible with 5.4.2 but I'm unclear exactly on how...

I haven't debugged SetParameterType very closely but I just noticed that it too has issues in TS 5.4.2

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • The funding will be given to active contributors.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@sindresorhus
Copy link
Owner

// @ifiokjr @Emiyaaaaa

@voxpelli
Copy link
Collaborator

How come the canary is not picking up on this? https://github.com/sindresorhus/type-fest/actions/workflows/ts-canary.yml

@voxpelli
Copy link
Collaborator

@trevorade Extending the tests to highlight this failure would be a good first step I think 👍

This is the second breakage in the 5.4-line, after #784

@trevorade
Copy link
Contributor Author

Hey @voxpelli, we were just updating our code repository in my company to 5.4.2 and this test started breaking so I figured I would send an early signal. I'm not really looking for contribution opportunities at the moment.

Here's the ConditionalKeys test rewritten in TS Playground: link (change the version to "5.3.3" to see the old behavior)

@shicks
Copy link

shicks commented Mar 13, 2024

How come the canary is not picking up on this? https://github.com/sindresorhus/type-fest/actions/workflows/ts-canary.yml

We don't use tsd in our internal fork, but have (programmatically) rewritten the tests to use a simpler typechecking assertion library. My guess about why your CI workflow didn't pick up the breakage is that it was using tsd for type checking, which has its own version of typescript built into it - so it's still effectively testing against the old version. (FWIW, this is the main reason we don't use tsd - we want a single typescript import to control all type checking everywhere).

@trevorade
Copy link
Contributor Author

trevorade commented Mar 13, 2024

For ConditionalKeys, looks like the fix is to just add -? and drop the NonNullable:

export type ConditionalKeys<Base, Condition> = {
  // Map through all the keys of the given base type.
  [Key in keyof Base]-?:
  // Pick only keys with types extending the given `Condition` type.
  Base[Key] extends Condition
  // Retain this key since the condition passes.
    ? Key
  // Discard this key since the condition fails.
    : never;

  // Convert the produced object into a union type of the keys which passed the conditional test.
}[keyof Base];

Fixes the tests on our end anyways.

@trevorade
Copy link
Contributor Author

Here's some debugging info for SerParameterType. Specifically, the issue appears to be with MergeObjectToTuple:

TS Playground link (change the version to 5.3.3 to see the difference)

This one may indicate a TypeScript bug

@trevorade
Copy link
Contributor Author

Simpler repro of the underlying issue.

TS Playground link (change the version to 5.3.3 to see the difference)

I'll see about logging an issue.

@Emiyaaaaa
Copy link
Collaborator

Emiyaaaaa commented Mar 14, 2024

Hi @trevorade

I did some test, If we focus only on the results of type A, it looks like 5.4.2 is correct?

But whatever this is a breaking change..

type Test<Tuple extends unknown[]> = {
	[K in keyof Tuple]: K
};

type A = Test<[a: number, b: string, c: Object, ...arguments: boolean[]]>
// v5.3.3 type A = [a: "0", b: "1", c: "2", ...arguments: "3"[]]
// v5.4.2 type A = [a: "0", b: "1", c: "2", ...arguments: number[]]

Playground Link

@trevorade
Copy link
Contributor Author

Looks like SetParameterType is failing for rest arguments due to this TSC PR: microsoft/TypeScript#57031

I haven't looked closer to see what the correct rewrite would be.

@sindresorhus
Copy link
Owner

SetParameterType should be fixed in https://github.com/sindresorhus/type-fest/releases/tag/v4.13.0

@trevorade
Copy link
Contributor Author

Thanks. I'll verify the fix on our end later today

@trevorade
Copy link
Contributor Author

trevorade commented Mar 19, 2024

Confirmed on our end that SetParameterType is fixed. Thx! [See update below]

ConditionalKeys should be easy to fix:
#831 (comment)

@trevorade
Copy link
Contributor Author

Apologies, SetParameterType is not quite fixed.

For this case in tsc 5.4.2:

// Test another define way
declare const test3: SetParameterType<typeof fn, [a: string, b: boolean]>;
expectType<(a: string, b: boolean, c: Object, ...args: boolean[]) => null>(test3);
test3('1', true, {}, true);

I'm seeing the type of test3 resolve to:

type test3Type = (_a: string, _b: boolean, _c: Object, ..._arguments: (string | boolean)[]) => null

You can ignore the difference in parameter names but the last rest parameter is resolving to (string | boolean)[] rather than the expected boolean[].

Here's a reproduction of this error in TS Playground.

@Emiyaaaaa
Copy link
Collaborator

Emiyaaaaa commented Mar 20, 2024

Sorry, I made a mistake that ignore the number key when input is an array. I'm fixing it right now.

It seems that the improvement of the testing system is more urgent.

@sindresorhus
Copy link
Owner

It seems that the improvement of the testing system is more urgent.

#837

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 a pull request may close this issue.

5 participants