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

⌨️ refactor UseFormMethods to arrow functions #2899

Merged
merged 21 commits into from
Sep 23, 2020

Conversation

Tymek
Copy link
Contributor

@Tymek Tymek commented Sep 14, 2020

resolves #2887

@Tymek Tymek marked this pull request as draft September 14, 2020 05:46
@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 15, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 6da53e6:

Sandbox Source
react-hook-form/react-hook-form: app Configuration

@Tymek
Copy link
Contributor Author

Tymek commented Sep 15, 2020

I got the main issue down, but I don't understand why TypeScript complains about generics in multiple invocation signatures while it was fine using overloaded functions :|

@bluebill1049
Copy link
Member

Thanks @Tymek I will take a look at it.

@bluebill1049 bluebill1049 marked this pull request as ready for review September 15, 2020 07:50
src/useForm.ts Outdated Show resolved Hide resolved
@bluebill1049
Copy link
Member

bluebill1049 commented Sep 15, 2020

actually @kotarella1110 did you had a similar issue before? I remember from one of the PR. it was to do with the type order. is this a result of cycle reference with Control?

@kotarella1110
Copy link
Member

kotarella1110 commented Sep 15, 2020

@bluebill1049 yes. I had a similar problem before (please see #1412 (comment)).
The solution was to change the order of the methods in the Control type.
The cause of this is not well understood...
I tested this and this passes... strange...

@bluebill1049
Copy link
Member

Thanks @kotarella1110 i will play with it a bit more.

@bluebill1049
Copy link
Member

I am having issue to get this one over the line as well, attached with help wanted tag, hopefully someone can figure out the reason and i will take a look at it again as well.

@bluebill1049 bluebill1049 added the help wanted Extra attention is needed label Sep 17, 2020
@bluebill1049
Copy link
Member

bluebill1049 commented Sep 17, 2020

It's a super crap this one, as unit tests pass locally.
Screen Shot 2020-09-17 at 8 43 57 pm

@bluebill1049
Copy link
Member

my 🧠 is fried... @kotarella1110 if you some free time, please help to take a look at this...

@Tymek
Copy link
Contributor Author

Tymek commented Sep 19, 2020

I did some more debugging and.. this is madness

maybe we can create matching Control from name, instead of trying to infer it?
what this type check does, is checking if right name exists on Control
inverting this will check if we can use passed Control with this name

@Tymek
Copy link
Contributor Author

Tymek commented Sep 19, 2020

I have no idea why tests passed locally, but I had the same problem. I sidestepped it with docker run -it -v ${PWD}:/srv -w /srv circleci/node:lts bash instead of loosing my sanity asking what are the differences between yarn test and npm test 😆

@bluebill1049
Copy link
Member

@Tymek I got my 🧠 fried by this issue... I couldn't find a clue why tests failed everyone and then, while e2e constantly(build) failed. I am not even sure it's our code or ts...

@JeromeDeLeon
Copy link
Contributor

This is the only time we could probably say that It works on my local. I have tried them out myself but no luck.

@bluebill1049
Copy link
Member

Thanks for trying to solve this "hard" problem @Tymek 🙏 ❤️

@Tymek
Copy link
Contributor Author

Tymek commented Sep 23, 2020

Your're welcome 😉 I'm glad that I can help

CI tests finally passed 🎉

@bluebill1049
Copy link
Member

oh wow! thank you! the power of OSS.

@@ -7,8 +7,7 @@ import skipValidation from './logic/skipValidation';
import isNameInFieldArray from './logic/isNameInFieldArray';
import { useFormContext } from './useFormContext';
import { VALUE } from './constants';
import { Control } from './types';
import { ControllerProps } from './types';
import { ControllerProps, FieldValues } from './types';
Copy link
Member

Choose a reason for hiding this comment

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

nice~

}: ControllerProps<TAs, TControl>) => {
const methods = useFormContext();
}: ControllerProps<TAs, TFieldValues>) => {
const methods = useFormContext<TFieldValues>();
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in theory changing this might break someone's build, but it's a niche case

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.

wow! amazing 🏆
looks so good! 💯
Thank you very much!!!!!

@bluebill1049
Copy link
Member

bluebill1049 commented Sep 23, 2020

Thank you very much @Tymek 🎖️ and @kotarella1110 for the review. 🙏

https://twitter.com/HookForm/status/1308908370284490753

@bluebill1049 bluebill1049 merged commit 3d713b3 into react-hook-form:master Sep 23, 2020
bluebill1049 added a commit that referenced this pull request Sep 26, 2020
@bluebill1049
Copy link
Member

Looks like this PR caused some issues... I am temporally rever it back 🙏

bluebill1049 added a commit that referenced this pull request Sep 26, 2020
* Revert "⌨️ refactor UseFormMethods to arrow functions (#2899)"

This reverts commit 3d713b3.

* attach useWatch type test

* include Control generic

* fix some minor type error

* fix readme
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ESlint TypeScript error unbound-method
4 participants