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: Nested deep partial with unknown #11373

Conversation

jonahallibone
Copy link
Contributor

@jonahallibone jonahallibone commented Jan 9, 2024

Hi!

In the recent 7.49.3 release, there was a change to make the DeepPartial compatible with unknown..

The side effect of this PR, however, was to make nested properties on objects no longer optional. I believe this is because the DeepPartial checked for extends object which inherently excludes all other types like string | null | undefined. I am not a TS wizard, but I think I am close with that assumption.

My change is to check for T extends never, which should solve this issue with unknown attempting to be solved in the previous PR but also preserving the types of non-object keys

An example of the issue I am referring to can be found here. The errors in questions will not appear on a version of this app with react-hook-form below 7.49.3. An example at 7.49.2 without the bug can be found here

I want to reiterate I am not a TS wizard, and I very well could be wrong with this solution. However, I am pretty sure we could label the behavior introduced in 7.49.3 as a bug.

Copy link

codesandbox bot commented Jan 9, 2024

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

@bluebill1049 bluebill1049 merged commit 75cab7f into react-hook-form:master Jan 9, 2024
7 checks passed
@jonahallibone jonahallibone deleted the jonahallibone/fix-nest-deep-partial branch January 9, 2024 22:38
@RulerOfCakes
Copy link
Contributor

RulerOfCakes commented Jan 11, 2024

Hi! I just found out about this PR. Sorry for the unintended bug in #11333.

To reinstate the problem, my previous PR didn't make nested properties on objects no longer optional for all object types. I was unaware that a union type containing both objects and non-object primitives didn't pass the extends object check, and thus it would have only failed in such cases like the example sandbox you provided in the PR.

However your solution seems to effectively revert the definition of DeepPartial back before #11333, as typically no type will satisfy extends never in this scenario. I have checked that this new definition will again cause errors with types containing unknown such as the previous example I gave in #11333, so it seems like we're back to square one here.

Before I upload a new PR, I'll post a new solution here to check for both cases:

export type ExtractObjects<T> = T extends infer U
  ? U extends object
    ? U
    : never
  : never;

export type DeepPartial<T> = {
  [K in keyof T]?: ExtractObjects<T[K]> extends never ? T[K] : DeepPartial<T[K]>;
};

This will be able to extract any object types that are present in a union(if T[K] happens to be one) and effectively apply DeepPartial for only those objects. Accounts for unknown as well since ExtractObjects<unknown> will evaluate to never too. Here is a quick sandbox link that I forked off to test compile errors. WDYT about this change? @bluebill1049 @jonahallibone

If all looks good, I'll upload a new PR with some type tests to account for all these cases we have found.

@bluebill1049
Copy link
Member

@RulerOfCakes please send a pr, would be good if we can include some type tests as well. 🙏

@jonahallibone
Copy link
Contributor Author

jonahallibone commented Jan 11, 2024

@RulerOfCakes Ah yes your solution is what I was aiming to do! I figured that infer would end up being part of the solution however in practice I've no experience with it. I thought from some (admittedly light) testing that my solution had somehow worked with unknown but I now see that it was more likely codesandbox having incredibly flaky lint warnings. Thanks for correcting me!

rafaelcalhau pushed a commit to rafaelcalhau/react-hook-form that referenced this pull request May 5, 2024
…hook-form#11373)

* fix: Nested deep partial with unknown

* reset editor files

* added api extractor
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.

None yet

3 participants