-
Notifications
You must be signed in to change notification settings - Fork 20
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: types and options #50
fix: types and options #50
Conversation
src/useHookFormMask.ts
Outdated
export function useHookFormMask<T extends FieldValues>(registerFn: UseFormRegister<T>) { | ||
return (fieldName: Path<T>, mask: Mask, options?: Options, registerOptions?: RegisterOptions) => { | ||
if (!registerFn) throw new Error('registerFn is required'); | ||
|
||
const { ref, ...restRegister } = registerFn(fieldName); | ||
const { ref, ...restRegister } = registerFn(fieldName, registerOptions); |
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 really dont like to use 3 params funcion. Not provide an good development experience. The secound funcion are an merge from Inputmask options and hook-form options. We dont need that.
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 see but unless I'm missing something, I think you're merging only the types, and the params belonging to react-hook-form
register are never really passed to it?
What I mean is, for example, it will auto complete options like setValueAs
, valueAsNumber
, valueAsDate
, which belong to register, but if if pass them, it will have no effect.
with the original code (after we fix the types import), TS won't complain about this
{...registerWithMask(
'price',
'brl-currency',
{ rightAlign: false, autoUnmask: true, setValueAs: (val) => (val.replace(',', '.')) }
)}
but the setValueAs
will have no effect
with the modified code, we could have something like
{...registerWithMask(
'price',
'brl-currency',
{ rightAlign: false, autoUnmask: true },
{ setValueAs: (val) => (val.replace(',', '.')) }
)}
It was the simplest way I could think without messing a lot with the original code.
Now let me explain how I use this specificaly. When I use a schema like Joi.number()
for my price
field, it validates numbers and strings that can be converted to numbers directly. So doing this is the simplest way to keep my schemas happy when I use commas for decimals, etc. I could also write a custom Joi validation, but passing setValueAs
to register is simpler.
I guess what I don't understand is why join the options if you can't do anything with the joined options?
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.
Uh! Of course, make all sense to me now. But... this pr will be a break change :/
I think we can sure that will be a minor change.
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.
What if we keep the merged types and pass only setValueAs
to registerFn
for now?
const { setValueAs } = options || {};
const { ref, ...restRegister } = registerFn(fieldName, { setValueAs });
Most of the other RegisterOptions
we can pass direct to the component anyway.
I also tried passing valueAsNumber
and valueAsDate
and TS started yelling at me, but I'm ok converting to Number inside onSubmit, etc.
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.
Uh, check my new change request.
src/useHookFormMask.ts
Outdated
const { ref, ...restRegister } = registerFn(fieldName); | ||
const { setValueAs } = options || {}; | ||
|
||
const { ref, ...restRegister } = registerFn(fieldName, { setValueAs }); |
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.
We can pass all the props to register, no problem.
const { ref, ...restRegister } = registerFn(fieldName, { setValueAs }); | |
const { ref, ...restRegister } = registerFn(fieldName, options); |
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.
Passing all props to Hook Form they works like a charm, ignoring the inputmask props.
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 didn't thought of passing everything to register as well :D
src/useHookFormMask.ts
Outdated
@@ -12,7 +12,9 @@ export function useHookFormMask< | |||
return (fieldName: Path<T>, mask: Mask, options?: Options & D) => { | |||
if (!registerFn) throw new Error('registerFn is required'); | |||
|
|||
const { ref, ...restRegister } = registerFn(fieldName); | |||
const { setValueAs } = options || {}; |
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.
You can remove that, of course.
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.
LGTM
## [3.3.4](3.3.3...3.3.4) (2023-08-29) ### Bug Fixes * types and options ([#50](#50)) ([3bc502e](3bc502e))
🎉 This PR is included in version 3.3.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Hi, while trying your lib I noticed the 3rd param for
registerWithMask
wasn't getting the types correctlyI'm not sure its the best way to fix it, but I did it importing
Inputmask
totypes.ts
Another thing is the options for
react-hook-form
were getting passed toinputmask
, so I separated them to a 4th paramHope those fixes are OK and thank you :)