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

KeysOfUnion: Fix assignability with keyof #1009

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions source/keys-of-union.d.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import type {UnionToIntersection} from './union-to-intersection';

/**
Create a union of all keys from a given type, even those exclusive to specific union members.

Expand Down Expand Up @@ -35,6 +37,6 @@ type AllKeys = KeysOfUnion<Union>;

@category Object
*/
export type KeysOfUnion<ObjectType> = ObjectType extends unknown
? keyof ObjectType
: never;
export type KeysOfUnion<ObjectType> =
// Hack to fix https://github.com/sindresorhus/type-fest/issues/1008
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way not hack?

Copy link
Collaborator Author

@som-sm som-sm Dec 17, 2024

Choose a reason for hiding this comment

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

Could be, but I couldn't figure out anything else.

Do you have any ideas or suggestions?

Copy link
Owner

Choose a reason for hiding this comment

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

I think it's fine to do this for now until we come up with a better solution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I agree!
And this is probably a bug, refer this comment.

Copy link
Collaborator Author

@som-sm som-sm Jan 20, 2025

Choose a reason for hiding this comment

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

Also, removed intersection with KeysOfUnions as it's not required after this fix, refer db0ce32.

keyof UnionToIntersection<ObjectType extends unknown ? Record<keyof ObjectType, never> : never>;
2 changes: 1 addition & 1 deletion source/set-optional.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,6 @@ export type SetOptional<BaseType, Keys extends keyof BaseType> =
// Pick just the keys that are readonly from the base type.
Except<BaseType, Keys> &
// Pick the keys that should be mutable from the base type and make them mutable.
Partial<HomomorphicPick<BaseType, Keys & KeysOfUnion<BaseType>>>
Partial<HomomorphicPick<BaseType, Keys>>
>
: never;
2 changes: 1 addition & 1 deletion source/set-readonly.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,6 @@ export type SetReadonly<BaseType, Keys extends keyof BaseType> =
BaseType extends unknown
? Simplify<
Except<BaseType, Keys> &
Readonly<HomomorphicPick<BaseType, Keys & KeysOfUnion<BaseType>>>
Readonly<HomomorphicPick<BaseType, Keys>>
>
: never;
2 changes: 1 addition & 1 deletion source/set-required.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export type SetRequired<BaseType, Keys extends keyof BaseType> =
// Pick just the keys that are optional from the base type.
Except<BaseType, Keys> &
// Pick the keys that should be required from the base type and make them required.
Required<HomomorphicPick<BaseType, Keys & KeysOfUnion<BaseType>>>
Required<HomomorphicPick<BaseType, Keys>>
>;

/**
Expand Down
51 changes: 50 additions & 1 deletion test-d/keys-of-union.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {expectType} from 'tsd';
import type {KeysOfUnion} from '../index';
import type {KeysOfUnion, UnknownRecord} from '../index';

// When passing types that are not unions, it behaves exactly as the `keyof` operator.

Expand Down Expand Up @@ -35,3 +35,52 @@ type Expected2 = 'common' | 'a' | 'b' | 'c';
declare const actual2: KeysOfUnion<Example2>;

expectType<Expected2>(actual2);

// With property modifiers
declare const actual3: KeysOfUnion<{a?: string; readonly b: number} | {a: number; b: string}>;
expectType<'a' | 'b'>(actual3);

// `KeysOfUnion<T>` should NOT be assignable to `keyof T`
type Assignability1<T, _K extends keyof T> = unknown;
Copy link
Collaborator

Choose a reason for hiding this comment

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

add test:

type Assignability1<T extends UnknownRecord, K extends keyof T> = unknown;
type Test1<T extends UnknownRecord> = Assignability1<T, KeysOfUnion<T>>; // should error

Copy link
Collaborator Author

@som-sm som-sm Dec 18, 2024

Choose a reason for hiding this comment

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

@Emiyaaaaa The test you suggested doesn’t seem to produce an error:

type Assignability5<T extends UnknownRecord, _K extends keyof T> = unknown;
type Test5<T extends UnknownRecord> = Assignability5<T, KeysOfUnion<T>>; // Doesn't error

However, adding slightly different constraints does produce the expected error:

type Assignability5<T extends Record<string, unknown>, _K extends keyof T> = unknown;
// @ts-expect-error
type Test5<T extends Record<string, unknown>> = Assignability5<T, KeysOfUnion<T>>; // Does error
type Assignability5<T extends object, _K extends keyof T> = unknown;
// @ts-expect-error
type Test5<T extends object> = Assignability5<T, KeysOfUnion<T>>; // Does error

Unsure what's happening here, any ideas?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have idea right now, maybe it's a bug of Typescript I'm not sure. I'll try to look into this interesting issue later

Copy link
Owner

Choose a reason for hiding this comment

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

tsd has some flaws with type checking, try assertions using expect-type instead: https://github.com/mmkal/expect-type

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sindresorhus These tests don't use either tsd or expect-type, they are just simple generics, like this:

type Assignability5<T extends Record<string, unknown>, _K extends keyof T> = unknown;
// @ts-expect-error
type Test5<T extends Record<string, unknown>> = Assignability5<T, KeysOfUnion<T>>; // Does error

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, maybe just add your tests, and then add type Assignability5<T extends UnknownRecord, _K extends keyof T> = unknown; type Test5<T extends UnknownRecord> = Assignability5<T, KeysOfUnion<T>>; // Doesn't error commented out with a comment that says that they don't produce error even though they should.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup makes sense, added all the above mentioned cases and some more, refer 2629884.

// @ts-expect-error
type Test1<T> = Assignability1<T, KeysOfUnion<T>>;

// `keyof T` should be assignable to `KeysOfUnion<T>`
type Assignability2<T, _K extends KeysOfUnion<T>> = unknown;
type Test2<T> = Assignability2<T, keyof T>;

// `KeysOfUnion<T>` should be assignable to `PropertyKey`
type Assignability3<_T, _K extends PropertyKey> = unknown;
type Test3<T> = Assignability3<T, KeysOfUnion<T>>;

// `PropertyKey` should NOT be assignable to `KeysOfUnion<T>`
type Assignability4<T, _K extends KeysOfUnion<T>> = unknown;
// @ts-expect-error
type Test4<T> = Assignability4<T, PropertyKey>;

// `keyof T` should be assignable to `KeysOfUnion<T>` even when `T` is constrained to `Record<string, unknown>`
type Assignability5<T extends Record<string, unknown>, _K extends KeysOfUnion<T>> = unknown;
type Test5<T extends Record<string, unknown>> = Assignability5<T, keyof T>;

// `keyof T` should be assignable to `KeysOfUnion<T>` even when `T` is constrained to `object`
type Assignability6<T extends object, _K extends KeysOfUnion<T>> = unknown;
type Test6<T extends object> = Assignability6<T, keyof T>;

// `keyof T` should be assignable to `KeysOfUnion<T>` even when `T` is constrained to `UnknownRecord`
type Assignability7<T extends UnknownRecord, _K extends KeysOfUnion<T>> = unknown;
type Test7<T extends UnknownRecord> = Assignability7<T, keyof T>;

// `KeysOfUnion<T>` should NOT be assignable to `keyof T` even when `T` is constrained to `Record<string, unknown>`
type Assignability8<T extends Record<string, unknown>, _K extends keyof T> = unknown;
// @ts-expect-error
type Test8<T extends Record<string, unknown>> = Assignability8<T, KeysOfUnion<T>>;

// `KeysOfUnion<T>` should NOT be assignable to `keyof T` even when `T` is constrained to `object`
type Assignability9<T extends object, _K extends keyof T> = unknown;
// @ts-expect-error
type Test9<T extends object> = Assignability9<T, KeysOfUnion<T>>;

// `KeysOfUnion<T>` should NOT be assignable to `keyof T` even when `T` is constrained to `UnknownRecord`
// type Assignability10<T extends UnknownRecord, _K extends keyof T> = unknown;
// The following line should error but it doesn't, this is an issue with the existing implementation of `KeysOfUnion`
// type Test10<T extends UnknownRecord> = Assignability10<T, KeysOfUnion<T>>;
Loading