type-fest icon indicating copy to clipboard operation
type-fest copied to clipboard

`PartialDeep`: recurse into objects in array without changing the array itself

Open yifanwww opened this issue 2 years ago • 6 comments

I saw this proposal https://github.com/sindresorhus/type-fest/issues/367 and the relevant context. I understand the need of recurseIntoArrays: false, but still get a little bit confused.

This https://github.com/sindresorhus/type-fest/issues/356#issuecomment-1022114231 talks about the situation about what problem recurseIntoArrays: true would cause.

Array types like (string|undefined)[] are thorny because JSON.parse(JSON.stringify(['a', undefined, 'b'])) becomes ['a', null, 'b'] on the other side and null is not assignable to undefined. This is where PartialDeep was correctly causing a type failure.

I agree with

In some sense I think PartialDeep is going a little too far. {foo: string[]} becomes {foo?: (string|undefined)[]} but the user probably just intended {foo?: string[]} which would be fine for serialization.

But I think we didn't consider all the type possibilities.

For serialization,

  • { foo: string[] } should become { foo?: string[] } instead of { foo?: (string | undefined)[] }
  • { foo: { bar: string }[] } should become { foo?: { bar?: string }[] } instead of { foo?: ({ bar?: string } | undefined)[] } or { foo?: { bar: string }[] }

Type-generically, there are 3 results that { foo: T[] } can become:

  1. { foo?: T[] }
  2. { foo?: PartialDeep<T, Options>[] }
  3. { foo?: PartialDeep<T | undefined, Options>[] }

I think No.2 is what most users want when serializing, not No.1.

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • The funding will be given to active contributors.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar

yifanwww avatar Nov 18 '22 03:11 yifanwww

// @bbrk24

sindresorhus avatar Nov 20 '22 12:11 sindresorhus

Huh, I see. I understand the request but I'm not sure how best to represent this in the options object.

bbrk24 avatar Nov 20 '22 23:11 bbrk24

Not sure if this is related, but I can't get PartialDeep to work with arrays when I want to re-assign values. Please have a look at the demo: reproduction-link

any advice maybe? :)

plehnen avatar Feb 21 '23 18:02 plehnen

@plehnen My initial reaction is that it seems to be a limitation of the language. After all, it works fine if you assign it to a temporary constant:

    const innerValue = impl[id1].innerArray[id2];
    assertIsDefined(innerValue);
    innerValue.id = 'a'; 

bbrk24 avatar Feb 21 '23 20:02 bbrk24

But how come that it works with any other "regular" type (see "payload2" example in the same link), but only with PartialDeep this error is introduced?

PS: my example is simplified. It is actually inside a vue ref, so assigning it to a temporary constant will lose its reactivity. Currently I solve this as I did in the example: Using Omit and re-assigning the "innerArray" with the new type. But I hate that I have to repeat myself and that TS wouldn't complain if someone would rename the originals "innerArray". Using PartialDeep seems so much more elegant - if only it would work the same. Weirdly enough, as both type should basically be the same, right?

plehnen avatar Feb 21 '23 22:02 plehnen

Heavy +1 for option 2. I think option 2 should be the default, option 1 defeats the point of a PartialDeep utility type in the first place since it arbitrarily stops at arrays which is very unintuitive, and there is IMO no need at all for option 3.

So with option 2 as the default the recurseIntoArrays parameter could be completely removed, or maybe provide a different parameter to allow for option 3 (but I don't think it would be needed).

Livven avatar Jun 20 '24 10:06 Livven