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

fix(core): add support to union types for DeepValue #724

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

irwinarruda
Copy link

Solves #691

Explanation

@Balastrong provided two issues in the example. Only one of those is a bug. Consider the following code:

type Text = { answer: string; textAnswer: string }
type Number = { answer: number }
type FormType = { questions: Array<Text | Number> }
  • When calling DeepValue<FormType, 'questions[0].textAnswer'> the result is unknown. This is a bug!
  • When calling DeepValue<FormType, 'questions[0].answer'> the result is number | string. This is not a bug because Typescript will never know the current state of the if checks.

Solution

As I explained on the issue page, the problem was solved by swapping the default typescript get function with a custom one.

Problems found along the way

Recently, the #713 and #717 were merged. From what I gathered, if there is a nullable union, all the properties from the object part will also be nullable. So for example:

type Obj = { a: { b: string } | null }
// DeepValue<Obj, 'a.b'> -> string | null

In order to create a standardized behavior, I decided to ask you some questions.

  • Why is it only null values? What about optional and undefined?
  • Shouldn't it work with deep nested values?
DeepValue<{ a: { b: { c: string } } | null }, 'a.b.c'>
// should be -> string | null but is only string
  • Should the object union work the same way? So for the first example I gave:
DeepValue<FormType, 'questions[0].textAnswer'>
// should be -> string | undefined instead of just string (which is the way I implemented it)

Please let me know if you would like to chat more about it. I recently joined the Tanstack Discord.

Copy link

nx-cloud bot commented Jun 20, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit b5701fd. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

Copy link
Member

@crutchcorn crutchcorn left a comment

Choose a reason for hiding this comment

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

This is awesome, thank you!

@crutchcorn crutchcorn mentioned this pull request Jun 27, 2024
5 tasks
@irwinarruda
Copy link
Author

irwinarruda commented Jun 28, 2024

The latest commit attempts to fix some of the issues discussed in the previous message.

It was clear that solving the Deep Nested Nullable problem would help with almost all other issues, so that's where I started.

After reading the code a bit more, the current implementation made sense and should've worked. I tried a lot of other approaches but none of them seemed to work at all. Then, I did the following test manually executing the DeepValue recursion:

type Obj2 = { user: { name: { first: string } } | null }
type normal1 = DeepValue<Obj2, 'user.name.first'> // string **Wrong! Should be string | null**
// The following is what should happen recursively. 
type manual1 = DeepValue<Obj2, 'user'> // { name: { first: string; } } | null **Correct**
type manual2 = DeepValue<manual1, 'name'> // { first: string; } | null **Correct**
type manual3 = DeepValue<manual2, 'first'> // string | null **The correct result we wanted**

I've had similar experiences, and I think it's something with typescript itself. The workaround was changing one of the if statements and adding my Get function (I don't actually know why it worked and that's why I think it's a typescript bug):

export type DeepValue<...> =
      TValue extends ReadonlyArray<any>
      ? ...
      : // Before, it was TValue extends Record<string | number, any> 
        TValue extends any
        ...
                ? TNullable extends true
                  // Get is needed here because it can handle anything whether it is an object or not.
                  ? Nullable<Get<TValue, TAccessor>>
                  : Get<TValue, TAccessor>
                : never

Though this implementation worked, it introduced a strange typescript error TS2589: Type instantiation is excessively deep and possibly infinite. - I say that because any editor I use does not pick up on that error consistently.

After trying a lot of things, I created a TDepth arg to DeepValue passing all tests.

I'm writing this to explain why I introduced a bit more complexity to the codebase. Also if it actually is a typescript bug maybe we should not do workarounds and instead open an issue since the code change seems very dangerous.

The only thing that did not work exactly as I wanted is described in the following test case:

type DoubleNestedNullableObjectCase = {
  mixed?: { mainUser: { name: 'name' } } | null | undefined
}
type DoubleNestedNullableObjectA = DeepValue<
  DoubleNestedNullableObjectCase,
  'mixed.mainUser'
>
// Notice how the `name` property is not nullable or undefined.
// It would be much harder to do that, but I wanted something like:
// assertType<{ name: 'name' | null | undefined } | null | undefined>(
assertType<{ name: 'name' } | null | undefined>(
  0 as never as DoubleNestedNullableObjectA,
)

@crutchcorn
Copy link
Member

We'd previously had issues with TDepth that we removed thanks to @chorobin. When we had TDepth, it caused major headaches that I'd like us to try to avoid merging back into the codebase if at all possible - as it leads to the possibly infinite issues.

@chorobin could you take a look at this issue and try to help us figure out what's going on and maybe open a GH issue with upstream TS if needed?

@irwinarruda
Copy link
Author

irwinarruda commented Jun 28, 2024

Also, it's important to clarify that the TDepth was the best way I found to solve the TS2589 error, but I don't think a "deep or infinite instantiation" was actually happening. For this case, It wouldn't matter if I put 10 or 20 as the maximum depth length. The whole Nullable thing feels hacky because the first implementation should've been working...

To help @chorobin, this is the updated code without the TDepth implementation.

/**
 * Infer the type of a deeply nested property within an object or an array.
 */
export type DeepValue<
  // The object or array in which we have the property whose type we're trying to infer
  TValue,
  // A string representing the path of the property we're trying to access
  TAccessor,
> =
  // If TValue is any it will recurse forever, this terminates the recursion
  unknown extends TValue
    ? TValue
    : // Check if we're looking for the property in an array
      TValue extends ReadonlyArray<any>
      ? TAccessor extends `[${infer TBrackets}].${infer TAfter}`
        ? /*
          Extract the first element from the accessor path (`TBrackets`)
          and recursively call `DeepValue` with it
          */
          DeepValue<DeepValue<TValue, TBrackets>, TAfter>
        : TAccessor extends `[${infer TBrackets}]`
          ? DeepValue<TValue, TBrackets>
          : TAccessor extends keyof TValue
            ? TValue[TAccessor]
            : TValue[TAccessor & number]
      : TAccessor extends `${infer TBefore}[${infer TEverythingElse}`
        ? DeepValue<DeepValue<TValue, TBefore>, `[${TEverythingElse}`>
        : TAccessor extends `[${infer TBrackets}]`
          ? DeepValue<TValue, TBrackets>
          : TAccessor extends `${infer TBefore}.${infer TAfter}`
            ? DeepValue<DeepValue<TValue, TBefore>, TAfter>
            : TAccessor extends string
              ?
                  | Get<TValue, TAccessor>
                  | (ApplyNull<TValue> | ApplyUndefined<TValue>)
              : never

The error doesn't happen when using DeepValue with constant strings. It only happens when the key is somehow inferred.

type BugType = { person?: { firstName: string } | null }
type t1 = DeepValue<BugType, 'person'> // { firstName: string; } | null | undefined as expected

type WithInfer<TKey extends DeepKeys<BugType>, TValue> = DeepValue<TValue, TKey>
type t2 = WithInfer<'person', BugType> // { firstName: string; } | null | undefined as expected

function bugInfer<TKey extends DeepKeys<BugType>>( _: TKey, __?: DeepValue<BugType, TKey>) {
  return void 0
}
bugInfer('person', { firstName: '' }) // { firstName: string; } | null | undefined but has the TS2589 error

@irwinarruda
Copy link
Author

I dreamt about a way to remove the TDepth argument. It actually worked! It's still a dangerous change that's worth looking more into.

@crutchcorn crutchcorn mentioned this pull request Jul 6, 2024
11 tasks
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.

2 participants