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.

Sign up for GitHub

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

Jump to bottom

[lodash] Fix GetFieldType indexing for numeric indexes #69457

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

[lodash] Fix GetFieldType indexing for numeric indexes #69457

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
634ec4e
fix: lodash GetFieldType indexing for numeric indexes
Apr 27, 2024
d8d95a4
chore(lodash): raise minimum TS version to 4.8
Apr 28, 2024
884b288
style(lodash): improve GetIndexedField indentation
Apr 28, 2024
d29d1da
test(lodash): add tests for GetFieldType numeric indexing
Apr 28, 2024
76df931
chore(sequelize): raise minimum TS version to 4.8
Apr 28, 2024
6d213dc
chore(yog2-kernel): raise minimum TS version to 4.8
Apr 28, 2024
1f486fc
chore(umzug): raise minimum TS version to 4.8
Apr 28, 2024
ff361cc
chore(sequelize-fixtures): raise minimum TS version to 4.8
Apr 28, 2024
a7807c3
chore: raise minimum TS version to 4.8
Apr 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 9 additions & 7 deletions types/lodash/common/object.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1022,15 +1022,17 @@ declare module "../index" {

type GetIndexedField<T, K> = K extends keyof T
? T[K]
: K extends `${number}`
? 'length' extends keyof T
? number extends T['length']
? number extends keyof T
? T[number]
: undefined
: K extends `${infer U extends number}`
Copy link
Collaborator

@rbuckton rbuckton May 2, 2024 •
edited

Choose a reason for hiding this comment

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

Does it matter that "1.23" and ".123" will work for this? It's fair if it doesn't matter since the previous code did not check against this, but it's possible to verify the result is an integer using this:

Suggested change
: K extends `${infer U extends number}`
: [K, K] extends [`${bigint}`, `${infer U extends number}`]

? "length" extends keyof T
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having a hard time following this due to inconsistent indentation. I this this line and below should be indented 1 level deeper.

Copy link
Author

Choose a reason for hiding this comment

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

Awesome, thanks for the quick feedback! I'll bump the minimumTypescriptVersion to 4.8 and write some unit tests for the change.

I'll also adjust the indentation level (for some reason prettier & eslint is acting up for me in the cloned repo).

? number extends T["length"]
? number extends keyof T
? T[number]
: undefined
: undefined
: undefined;
: U extends keyof T
? T[U]
: undefined
: undefined;

type FieldWithPossiblyUndefined<T, Key> =
| GetFieldType<Exclude<T, undefined>, Key>
Expand Down