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

Utility type KnownKeys<T> broken on TypeScript 4.3 #951

Closed
4 of 10 tasks
lokshunhung opened this issue Jun 1, 2021 · 9 comments
Closed
4 of 10 tasks

Utility type KnownKeys<T> broken on TypeScript 4.3 #951

lokshunhung opened this issue Jun 1, 2021 · 9 comments
Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:minor TypeScript-specific
Milestone

Comments

@lokshunhung
Copy link
Contributor

lokshunhung commented Jun 1, 2021

Description

The following code no longer compiles after upgrading from TypeScript 4.2.4 to 4.3.2

(Click here to expand)
import { App } from "@slack/bolt";
const app = new App();
app.command("/say_hi", async ({ ack, respond }) => {
    await ack();
    await respond({
        blocks: [{ type: "section", text: { text: "hi", type: "plain_text" } }],
    //  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    // error TS2345: Argument of type '{ blocks: { type: string; text: { text: string; type: string; }; }[]; response_type: "ephemeral"; }' is not assignable to parameter of type 'string | RespondArguments'.
    //  Object literal may only specify known properties, and 'blocks' does not exist in type 'RespondArguments'.    });
        response_type: "ephemeral",
    });
});

I have narrowed down the problem to the utility type KnownKeys<T> in /src/types/helpers.ts.

type type KnownKeys<T> = {
    [K in keyof T]: string extends K ? never : number extends K ? never : K;
} extends { [_ in keyof T]: infer U }
    ? U
    : never;

type Foo = {
    foo: "Foo";
    bar: "Bar";
    [key: string]: any;
};

type Result = KnownKeys<Foo>;
// type of `Result` is `never` in TypeScript 4.3.2
// type of `Result` is `"foo" | "bar"` in TypeScript 4.2.4

KnownKeys<T> failing to resolve the keys has caused RespondArguments in /src/types/utilities.ts to omit the property "blocks" by calling Pick with never as the 2nd type argument:

type RespondArguments = Pick<
  ChatPostMessageArguments,
  Exclude<KnownKeys<ChatPostMessageArguments>, 'channel' | 'text'> // <-- KnownKeys<T> fails here
> & { /* ... */ };

...which in turn causes RespondFn to report an error when "blocks" is included in respond({ /* ... */ })

What type of issue is this? (place an x in one of the [ ])

  • bug
  • enhancement (feature request)
  • question
  • documentation related
  • example code related
  • testing related
  • discussion

Requirements (place an x in each of the [ ])

  • I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've searched for any related issues and avoided creating a duplicate issue.

Bug Report

Filling out the following details about bugs will help us solve your issue sooner.

Reproducible in:

package version: 3.3.0

node version: 15.12.0

OS version(s): OSX 11.3.1

Steps to reproduce:

(See description)

Also, /types-tests/utilities.test-d.ts should show an error after upgrading to TypeScript 4.3 (See attachment)

Expected result:

KnownKeys<T> should work properly in TypeScript 4.3.
Maybe add a test case to cover type Result = KnownKeys<Foo>; should not infer type never.

Actual result:

KnownKeys<T> infers never in TypeScript 4.3.

Attachments:

image

@gitwave gitwave bot added the untriaged label Jun 1, 2021
@seratch seratch added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:minor TypeScript-specific and removed untriaged labels Jun 1, 2021
@seratch seratch added this to the 3.4.0 milestone Jun 1, 2021
@seratch
Copy link
Member

seratch commented Jun 1, 2021

Hi @lokshunhung, thank you very much for taking the time to write this clear and thorough bug report! I've confirmed this is reproducible in my environment too.

@lokshunhung
Copy link
Contributor Author

I've looked into the implementation of KnownKeys<T> a bit, the current implementation (after formatting by hand):

type KnownKeys<T> = {
  [K in keyof T]: string extends K ? never :
                  number extends K ? never :
                  K;
} extends { [_ in keyof T]: infer U }
  ? U
  : never;

The first clause reads as "Given a type T, for each key as K in T, if string extends K or number extends K then return never, else return K", the second clause reads as "Given the result object type of the first clause (which is an object type { [Key in keyof T]: Key | never }), return a union type of the values of this result object type.

I think this implementation can be further simplified to the following for better readability:

type KnownKeys<T> = {
  [K in keyof T]: string extends K ? never :
                  number extends K ? never :
                  K;
}[keyof T]

...which essentially does the same thing, doing the string extends and number extends check and returning K, then using keyof T to index the intermediate object (but now without needing infer!)

@lokshunhung
Copy link
Contributor Author

Unfortunately after refactoring KnownKeys<T>, it still does not work in TypeScript 4.3 (because it is doing the same thing).

Digging further in the codebase, I found that KnownKeys<T> is only used in type SayArguments and type RespondArguments in /src/types/utilities.ts.

A solution to the current issue that I could think of at the moment, is to completely remove KnownKeys<T>, since it is only used in these two places, and is not exported as public API of the library.

Let me know your thoughts :)

@seratch
Copy link
Member

seratch commented Jun 1, 2021

@lokshunhung Thanks for sharing your insights!

Yes, I was also thinking that, as a last option, we may have to stop using KnownKeys and re-define SayArguments and RespondArguments by duplicating most parts of ChatPostMessageArguments.

@lokshunhung
Copy link
Contributor Author

@seratch I created a PR (#953) and appreciate if you could take the time to review it :)

@seratch
Copy link
Member

seratch commented Jun 2, 2021

@lokshunhung Reviewed it and it looks great to me! You hero, thanks so much 👏

@seratch seratch closed this as completed in 0ec4617 Jun 2, 2021
@lokshunhung
Copy link
Contributor Author

lokshunhung commented Jun 9, 2021

@seratch I think I found a solution to use a generic utility type.

TLDR: Let's keep the current solution as-is, because the trade-off from a generic solution is not worth it.

This can be achieved using key remapping in mapped types to check the types of keys inside index signature with conditional types, instead of using a conditional type as a object property. This solves the issue of TS4.3 seems to evaluate the types of keys too early (I believe) and causes the original utility type to fail.

The final code can be refactored to:

export type SayArguments = Omit<
  OmitIndexSignature<ChatPostMessageArguments>,
  'channel'
> & { /* ... */ };

export type RespondArguments = Omit<
  OmitIndexSignature<ChatPostMessageArguments>,
  'channel' | 'text'
> & { /* ... */ };

And the utility type looks like this:

type OmitIndexSignature<T> = {
  [K in keyof T as string extends K ? never : number extends K ? never : K]: T[K];
};

This reads as "Given a type T, for each key as K in T, if string extends K or number extends K then use never as the type for index, else use K as the type for index; then use T[K] to get the corresponding property type of the computed index key from T". It is essentially combining the step of computing known-keys and picking them from the original object into a single step. Moving the conditional check from the object property to the index signature seems to solve the problem where the behaviour of computing types in object properties in TS4.3 differs from prior versions.

However, the downside is that key remapping is a feature that is only available after TS4.1, which means causing problems to users using an older version of TypeScript if this solution is used.

So I guess it's best in the interest of the majority of users to use the more verbose way and "pick" all the properties of ChatPostMessageArguments to support a wider range of TypeScript versions. But if in the future this library decides to drop support for TypeScript versions that are too old, I hope someone finds this comment so they can trace back to this conversation when refactoring 😄

@seratch
Copy link
Member

seratch commented Jun 9, 2021

@lokshunhung It's great to know that there is a new way to achieve the same but I agree that this project won't go with the approach considering the supported TS versions. Thanks a lot for sharing this insightful knowledge!

@schoening
Copy link

schoening commented Nov 5, 2021

@lokshunhung thats the only implementation that worked for me, thank you :)
The other KnownKeys examples returned a never type on my machine unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:minor TypeScript-specific
Projects
None yet
Development

No branches or pull requests

3 participants