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

feat(get): add get #232

Merged
merged 8 commits into from
Jul 18, 2024
Merged

feat(get): add get #232

merged 8 commits into from
Jul 18, 2024

Conversation

piquark6046
Copy link
Contributor

@piquark6046 piquark6046 commented Jul 18, 2024

Summary

This PR addresses #215 and #91 by implementing the get function.

Benchmark

Screenshot from 2024-07-18 17-11-36

Copy link

vercel bot commented Jul 18, 2024

@piquark6046 is attempting to deploy a commit to the Toss Team on Vercel.

A member of the Team first needs to authorize it.

@piquark6046 piquark6046 marked this pull request as ready for review July 18, 2024 09:10
@piquark6046 piquark6046 requested a review from raon0211 as a code owner July 18, 2024 09:10
@de-novo
Copy link
Contributor

de-novo commented Jul 18, 2024

@piquark6046 I would like to suggest an improvement to the types in this PR. The current issue is that when the path depth exceeds one level, the return type is inferred as unknown. It would be beneficial if the return value of the get function could be clearly inferred.
스크린샷 2024-07-18 오후 7 39 21

Below is a type example that addresses this issue. This is a type I have used personally, and while it may not work perfectly in all situations, it should serve as a useful reference.

type Split<STR extends string, SEP extends string> = string extends SEP
  ? string[]
  : STR extends `${infer A}${SEP}${infer B}`
    ? [A, ...(B extends '' ? [] : Split<B, SEP>)]
    : SEP extends ''
      ? []
      : [STR];

type ReplaceAll<T extends string, A extends string, B extends string> = T extends `${infer C}${A}${D}`
  ? `${C}${B}${ReplaceAll<D, A, B>}`
  : T;

export type StringPathToArray<T extends string> = Split<ReplaceAll<ReplaceAll<T, '[', '.'>, ']', ''>, '.'>;
type At<T extends object | any[], K extends Array<string | number>> = K extends [
  infer F extends string | number,
  ...infer R extends Array<string | number>,
]
  ? F extends keyof T
    ? AtHelper<T[F], R>
    : F extends `${infer N extends number}`
      ? N extends keyof T
        ? AtHelper<T[N], R>
        : never
      : never
  : T;

type AtHelper<T, R> = T extends object | any[] ? At<T, R> : T;
interface Test {
  a: { b: { c: number[] } };
}

type test = At<Test, ['a', 'b', 'c', 0]>; // number

interface Test2 {
  a: Array<{ b: { c: number[] } }>;
}

type test2 = At<Test2, ['a', 0, 'b', 'c']>; // number[]

@piquark6046
Copy link
Contributor Author

@de-novo Can you please describe about D in your code snippet?

raon0211
raon0211 previously approved these changes Jul 18, 2024
Copy link
Collaborator

@raon0211 raon0211 left a comment

Choose a reason for hiding this comment

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

Hey, thanks for your contribution!

We were in the process of working on the get function. Let me combine our version with the one you implemented.

@raon0211
Copy link
Collaborator

Hello @de-novo , thanks for your suggestion!

Unfortunately, one of our core values of our library is simple implementation. I understand that your provided type helps our type safety, but I guess the type is too complicated to merge. We might open a new issue about this.

Let me merge this pull request, and let's discuss about correct types in the other issue if needed.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.49%. Comparing base (404d4a5) to head (95c5f54).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #232      +/-   ##
==========================================
+ Coverage   99.47%   99.49%   +0.01%     
==========================================
  Files          92       93       +1     
  Lines         570      590      +20     
  Branches      105      111       +6     
==========================================
+ Hits          567      587      +20     
  Misses          3        3              

@raon0211 raon0211 merged commit a186e5d into toss:main Jul 18, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants