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

✨ Add support for generic components using FieldPathWithValue #6562

Merged
merged 21 commits into from Oct 2, 2021
Merged

✨ Add support for generic components using FieldPathWithValue #6562

merged 21 commits into from Oct 2, 2021

Conversation

julienfouilhe
Copy link
Contributor

@julienfouilhe julienfouilhe commented Sep 16, 2021

This seems like a better and much simpler API for the support of generic components than #6466

If the expected type is not specified to useController/Controller, then all possible types from the FieldValues are returned.

You can look at the test in useController.test.tsx for how it can be used.

@julienfouilhe
Copy link
Contributor Author

julienfouilhe commented Sep 16, 2021

cc @bluebill1049 @kotarella1110 and @barrymay since you were tagged as reviewers on my other PR.

I could not find a way to keep the tests the same, I had to explicitly pass the types to userController/Controller. I think there may be a way to infer everything.

Edit: Nvm, found a way! :)

bluebill1049
bluebill1049 previously approved these changes Sep 16, 2021
Copy link
Member

@bluebill1049 bluebill1049 left a comment

Choose a reason for hiding this comment

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

LGTM.

@bluebill1049
Copy link
Member

let's wait for @barrymay and @kotarella1110's review feedback 🙏

barrymay
barrymay previously approved these changes Sep 16, 2021
Copy link
Member

@kotarella1110 kotarella1110 left a comment

Choose a reason for hiding this comment

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

Amazing!!! 🙌
Please check my comments.

src/types/controller.ts Outdated Show resolved Hide resolved
src/types/controller.ts Show resolved Hide resolved
@julienfouilhe
Copy link
Contributor Author

julienfouilhe commented Sep 28, 2021

@bluebill1049 Done! Sorry for the force-push which dismissed @kotarella1110 review! I usually rebase instead of merge so I was confused for a moment. But all good now!

The diff of the merge commit is weird but I just moved a type I had called ControllerFieldErrors and that was below ControllerFieldState above it, and renamed it ControllerFieldError which was the name used in one of the latest commits on master.

I also added expectations in my tests as @barrymay suggested. Since they were tests that just ensured everything was typed correctly, I just put basic expectations.

@bluebill1049
Copy link
Member

Thanks for everyone's contribution and review. Let's do another patch release for all the bug fixes before release this as a new minor update. I will merge that once the patch goes out, there are still few small issues yet to be resolved.

@bluebill1049
Copy link
Member

Just some thoughts, if this is a pattern that we all agree on, perhaps we can introduce something similar to useFieldArray? any thoughts.

@julienfouilhe
Copy link
Contributor Author

@bluebill1049 Definitely something that can be applied to it too!
I don't know how people create generic components. I mainly use Controller thus why I implemented it there, but if others use extensively useFieldArray (which I haven't used much for now) it could be useful to make that change there later too!

@bluebill1049 bluebill1049 merged commit f5a2519 into react-hook-form:master Oct 2, 2021
DeepPartialImpl<UnionLike<T>>,
FieldError
>;
type ControllerFieldError<
Copy link
Member

Choose a reason for hiding this comment

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

I didn't pay close enough attention here, @kotarella1110 this was completely changed after the merge.

Copy link
Member

Choose a reason for hiding this comment

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

which potentially lead to this: #6679

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bluebill1049 I don't understand the linked issue is talking about versions before this was merged right?

Copy link
Member

Choose a reason for hiding this comment

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

Screen Shot 2021-10-05 at 6 41 06 pm

i am just digging around at the moment, see if i can find a fix for it.

Copy link
Member

Choose a reason for hiding this comment

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

DeepMapImpl<DeepPartialImpl<UnionLike<TFieldValues>>, FieldError>

vs

DeepMap<DeepPartial<UnionLike<TFieldValues>>, FieldError>

Copy link
Member

Choose a reason for hiding this comment

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

still try to get my head around it.

Copy link
Member

Choose a reason for hiding this comment

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

ok confirmed the latest definitely related for causing the issue, it could be related to the user's typescript version as well, as i couldn't reproduce the issue at the lib level but codesandbox with CRA, after omit TResult and the issue went away.

Copy link
Member

Choose a reason for hiding this comment

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

@bluebill1049
Copy link
Member

I have to revert this one back guys, i don't have quick solution yet and it probably impact a lot of users.

@bluebill1049
Copy link
Member

we can come back to this, I am sorry to roll this back but it's better not impact more users.

bluebill1049 added a commit that referenced this pull request Oct 5, 2021
@bluebill1049
Copy link
Member

I think we still want this awesome feature.

@barrymay
Copy link
Contributor

barrymay commented Oct 5, 2021

switch to 17.0.0

For clarity, switch RHF to 7.17.0 to see the issue. As you said, very strange this didn't come up in the test run. I agree this feature is very much worth trying to get right (and also worth ensuring our test runs can catch this too)

@slbls
Copy link

slbls commented Oct 7, 2021

I'm new to contributing but would love to help out with this. What can I do to get started on making this viable again?

@bluebill1049
Copy link
Member

bluebill1049 commented Oct 7, 2021

thanks @slbls once this one is unblocked, we are pretty confident to bring this feature back.

@exneval
Copy link
Contributor

exneval commented Oct 8, 2021

hope this can be reimplemented

@bluebill1049
Copy link
Member

@julienfouilhe can you push and open this PR again? I think we can get this one back now.

@bluebill1049
Copy link
Member

bluebill1049 commented Oct 9, 2021

let's run that in the next tag for a while, let us know how did you guys go.

react-hook-form@7.18.0-next.0

https://github.com/react-hook-form/react-hook-form/releases/tag/v7.18.0-next.0

going to play more cautious with any TS-related update.

@barrymay
Copy link
Contributor

barrymay commented Oct 9, 2021

See my note here about this issue: #6753 (comment)

bluebill1049 added a commit that referenced this pull request Oct 22, 2021
…#6753)

* Revert "Revert "✨  Add support for generic components using `FieldPathWithValue` (#6562)" (#6717)"

This reverts commit 08883bb.

* keep FieldError simple

* fix with error object with only FieldError type

* 7.18.0-next.0

* include test case for controller with type check
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

6 participants