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

Add Paths type #741

Merged
merged 4 commits into from
Nov 7, 2023
Merged

Add Paths type #741

merged 4 commits into from
Nov 7, 2023

Conversation

Emiyaaaaa
Copy link
Collaborator

@sindresorhus
Copy link
Owner

It was suggested in the issue that the Get type could use this. Do you think so?

sindresorhus
sindresorhus requested changes Nov 5, 2023
View reviewed changes
source/paths.d.ts Outdated Show resolved Hide resolved
source/paths.d.ts Outdated Show resolved Hide resolved
source/paths.d.ts Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
source/paths.d.ts Show resolved Hide resolved
@Emiyaaaaa
Copy link
Collaborator Author

It was suggested in the issue that the Get type could use this. Do you think so?

Get accept 'string' and 'string[]':

  1. argument 'string' is like hits.hits[0]._source.name, but Paths current result like hits.hits.0._source.name
    I'm having some trouble:
    Is result of Paths wrong?
    or should add a option to choose the handling of array?
    or path in Get and Paths just are not same thing?
  2. argument 'string[]' is like ['hits','hits', '0', '_source', 'name'], it can replace with Split<Paths<OBJECT>, '.'>

And this is a broken change if replace type directly, so we should think about backward compatibility, example we can use LiteralUnion<Paths<T>, string> to replace string in Get

@Emiyaaaaa
Copy link
Collaborator Author

Emiyaaaaa commented Nov 7, 2023 •
edited

Is result of Paths wrong?
or should add a option to choose the handling of array?

Should resolve this question in current PR, need your help~ @sindresorhus

I perfer add a option to choose the handling of array, this is more flexible

@sindresorhus
Copy link
Owner

I forgot that Get accepts [0]. Then it doesn't make sense to use Paths. We can consider adding an option for Paths to also generate [0] paths, but that shouldn't be part of this pull request and we should discuss it in an issue first.

@Emiyaaaaa
Copy link
Collaborator Author

Emiyaaaaa commented Nov 7, 2023 •
edited

Changes:

  • resolved review
  • added support for tuple with spread item
  • return '0' | 0 for Paths<[string]>
  • return never for the type witch haven't any paths, like empty object and empty array.

@sindresorhus sindresorhus merged commit 996171b into sindresorhus:main Nov 7, 2023
6 checks passed
@sindresorhus
Copy link
Owner

Nice work 🎉

Emiyaaaaa
Emiyaaaaa commented Nov 8, 2023
View reviewed changes
source/paths.d.ts
? T[Key] extends UnknownRecord | UnknownArray
? (
IsNever<Paths<T[Key]>> extends false
// If `Key` is number, return `Key | `${Key}``. Because both `array[0]` and `array['0']` are work.
// If `Key` is a number, return `Key | `${Key}``, because both `array[0]` and `array['0']` do not work.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this a mistake? @sindresorhus

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, fixed.

@voxpelli voxpelli mentioned this pull request Apr 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sindresorhus sindresorhus Awaiting requested review from sindresorhus

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature Request: add Paths type
2 participants
@Emiyaaaaa @sindresorhus