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 react native TS types for register - related to #369 #381
Conversation
…t arg of register
- similar to how it's done in Web, but allowing separate name prop setting since native elements have no name - this also allows a register definition to override the name if that's desired
…ive model where name comes second
src/useForm.ts
Outdated
// For JSX registry | ||
// React-Native (Element has no name prop, so it must be passed in on teh validateRule) | ||
function register<Element>( | ||
validateRule: ValidationOptions & { name: string }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it's worth to turn { name: string }
into a type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair and done (albeit maybe a bit overdone, I won't fight it :) )
): ((ref: Element | null) => void) | void { | ||
if (typeof window === UNDEFINED || !refOrValidateRule) return; | ||
|
||
if (validationOptions && typeof validationOptions.name === 'string') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we only support
register({ name: 'xxx' }, { required: true })
is this try to support
register({ name: 'xxx', required: true })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exactly - because when you pass in a ref to register from react native, that ref will not have a name on it (because react-native doesn't support the name property on those text elements)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good 👍 I guess we can do the same for the web as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes - with this exact change (and I noted it in the comment) if you put the name in the second argument, it will override the name put on the element. You likely wouldn't do it, but it maybe helpful in a rare case.
thanks very much @barrymay for investigating on this issue :) |
@barrymay merge whenever you are ready :) |
As a follow-up to my investigation on #369, I don't see a problem using react-native import-wise with react-native. In terms of the actual implementation, it's only using types from react-native, not any implementation.
That said, in doing a local test, I did see a case where using react-native type elements in TS causes a problem, because react-native's elements don't support a name property. (I tried following the demo from https://react-hook-form.com in TS and ran into this case)
So I added an extra name property model to the register call
Here's an example of usage with the method - the other model using jsx is equivalent to what's on on react-hook-form.com
(technically, it may be possible to get rid of the ref here altogether since we're manually setting the value in the onChangeText call, but it appears sufficient for now)
@bluebill1049 @stramel feel free to give any feedback!