-
-
Notifications
You must be signed in to change notification settings - Fork 652
Fix: add index signature support for Paths
#1191
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
base: main
Are you sure you want to change the base?
Conversation
Path returning never for index signature typesPaths returning never for index signature types
43641aa to
5439e61
Compare
**Changes**
- fix: `Paths` no longer returns `never` for index signature types.
- refactor: Removed unnecessary conditions and repetition in `Paths`.
- refactor: Updated `LiteralUnion` to return `(string & {})` instead of `(string & Record<never, never>)`.
- add: Introduced `EmptyArray` and `IsEmptyArray` types (internal).
- fix: `IsEmptyObject` no longer returns `never` for `never`.
**Tests**
- add: Added tests for new features and bug fixes.
- update: Updated existing tests to reflect changes.
5439e61 to
1205f53
Compare
Can you elaborate on why that is better? And it needs a test, if possible. |
@sindresorhus Its just a visual improvement that does not change the functionality of the type. I test it using this example and some others should I add a test file? type LiteralUnion<LiteralType, BaseType extends Primitive> = LiteralType | (BaseType & Simplify<{}>);
// Pass
expectType<LiteralUnion<'foo', string>>({} as (string & {}) | 'foo');
expectType<LiteralUnion<'foo', string>>({} as (string & Record<never, never>) | 'foo');
// Fail
expectType<string>({} as (string & {}) | 'foo'); |
b084321 to
eaef970
Compare
eaef970 to
f045ccb
Compare
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.
Maybe we could return an uncollapsed union like
(string & {}) | `${string}.a` | `${string}.b`, but we’d have to decide in what situations we should return an uncollapsed union.
As mentioned here, just adding a check for string wouldn't be sufficient, because there would be other cases as well where the result would get collapsed.
The existing implementation fails in the following cases:
-
type T = Paths<{[x: Uppercase<string>]: {a: string}; C: {a: string}}>; //=> Uppercase<string> | `${Uppercase<string>}.a`
C.ais lost, result should have probably been:Uppercase<string> | (`${Uppercase<string>}.a` & {}) | "C.a" -
type T = Paths<{a: {[x: string]: number} | {b: number}}>; //=> "a" | `a.${string & {}}`
a.bis lost, result should have probably been'a' | (`a.${string}` & {}) | 'a.b'.
There would likely be more cases, hence it needs to be thought out properly.
Also, it'd be much easier to review if the PR only fixed the bug and not introduced a bunch of refactoring changes along with it.
source/paths.d.ts
Outdated
| [Key in keyof T]: | ||
| Key extends string | number // Limit `Key` to string or number. | ||
| ? ( | ||
| Options['bracketNotation'] extends true | ||
| ? IsNumberLike<Key> extends true | ||
| ? `[${Key}]` | ||
| : (Key | ToString<Key>) | ||
| : Options['bracketNotation'] extends false | ||
| // If `Key` is a number, return `Key | `${Key}``, because both `array[0]` and `array['0']` work. | ||
| ? (Key | ToString<Key>) | ||
| : never | ||
| ) extends infer TranformedKey extends string | number ? | ||
| [Key in keyof T]: Key extends PathableKeys // Limit `Key` to string or number. | ||
| ? (Options['bracketNotation'] extends true | ||
| ? IsNumberLike<Key> extends true | ||
| ? `[${Key}]` | ||
| : (Key | ToString<Key>) | ||
| : (Key | ToString<Key>) // If `Key` is a number, return `Key | `${Key}``, because both `array[0]` and `array['0']` work. | ||
| ) extends infer TranformedKey extends PathableKeys ? |
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.
Looks like an irrelevant change.
| /** | ||
| Represents pathable keys, the `string` or `number` value. | ||
| */ | ||
| type PathableKeys = string | number; |
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.
Ideally, this should not be part of this PR. It introduces a bunch of refactoring changes, which makes it harder to review the actual bug-related changes.
| ) | ( | ||
| Options['bracketNotation'] extends false | ||
| ? `${TranformedKey}.${SubPath}` | ||
| : never |
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.
Why remove Options['bracketNotation'] extends false conditional in this PR?
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.
if its not true -> its false there is no in bettwen. bracketNotation extends boolean !
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.
Why do that in this PR, what's the scope of this PR?
source/paths.d.ts
Outdated
| ? IsNumberLike<Key> extends true | ||
| ? `[${Key}]` | ||
| : (Key | ToString<Key>) | ||
| : Options['bracketNotation'] extends false |
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.
Why remove Options['bracketNotation'] extends false conditional in this PR?
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.
Why is the purpose of it in the first place?
**Changes** - split `Paths` into smaller types. - introduce `FilterWideTypes` to catch wide types. - add `Key` condtions to manipulate wide types keys. - refactor types structure, improving types logic and remove unnessasary condition.
dca0545 to
bc12bfe
Compare
|
@sindresorhus @som-sm Hey guys, so I wanted to discuss the new changes. declare const edge1: Paths<{a: {[x: string]: number} | {b: number}}>;
expectType<'a' | 'a.b' | (`a.${string}` & {})>(edge1); // Pass
declare const edge2: Paths<{[x: Uppercase<string>]: {a: string}; C: {a: string}}>;
expectType<'C' | 'C.a' | (Uppercase<string> & {}) | (`${Uppercase<string>}.a` & {})>(edge2); // Pass
declare const stringRecord: Paths<Record<string, {a: number; b: number}>>;
expectType<(string & {}) | `${string}.a` | `${string}.b`>(stringRecord); // Pass
declare const templateRecord: Paths<Record<`on${string}`, {a: number; b: number}>>;
expectType<(`on${string}` & {}) | (`on${string}.a` & {}) | (`on${string}.b` & {})>(templateRecord); // Pass
declare const complexRecord: Paths<Record<number, {a: number; b: number}> & DeepObject>;
expectType<number | `${number}` | 'a' | (`${number}.b` & {}) | (`${number}.a` & {}) | 'a.b' | 'a.b2' | 'a.b3' | 'a.b.c' | ${`a.b2.${number}` & {}} | 'a.b.c.d'>(complexRecord); // PassExplaination At the moment this it the best result so far, but maybe I can find a way to include the
I do apologize for the extra refactoring and changes, but I couldn't stand the compact type and all-in-one approach that was originally used. A best practice is to split the types into reusable pices that do one job. I apologize again 🙏for the mess. |
Paths returning never for index signature typesPaths
We are more than happy to have refactoring and readability improvements. What we are trying to tell you is that those changes are better done separately as they make it hard to see the actual changes. |
|
Okay captain, lets keep it for another PR. |
Closes #1190
Changes
Pathsaccepts index signature types and output all possible paths.Paths.LiteralUnionto return(string & {})instead of(string & Record<never, never>).EmptyArrayandIsEmptyArraytypes (internal).OwnKeystypes that extract own proprety keys from object/arrays.Tests