Upgrade RHF to v7.11.0 #3043
Upgrade RHF to v7.11.0 #3043
Conversation
| const tagProps = inputTagProps(props) as T & { | ||
| onChange?: React.ChangeEventHandler<T> | ||
| onBlur?: React.FocusEventHandler<T> | ||
| } |
There was a problem hiding this comment.
I'm not happy about this type casting but only have few hours today, open for any better suggestions
|
@callingmedic911 Fyi, cherry-picked your |
|
@LBrian thanks for your patience! Having a look at this now; will be my focus for the rest for the week. |
| const onBlur: React.FocusEventHandler<T> = (event) => { | ||
| handleBlur(event) | ||
| tagProps?.onBlur?.(event) | ||
| } | ||
| const onChange: React.ChangeEventHandler<T> = (event) => { | ||
| handleChange(event) | ||
| tagProps?.onChange?.(event) | ||
| } |
There was a problem hiding this comment.
Agree with you here that react hook form's blur/change handlers should be invoked first. Also falls in line with their documentation (might have to scroll down a bit to see the screenshot below):
|
@LBrian this is awesome; thanks for hacking on the framework with us (and most of all your patience!) 🚀 I think this is ready to go as is, and if we need to release soon we'll just merge it. But I think we still have a few days before the release at least, and there's one other thing we want to do and should've done when we upgraded to v6, and that's use react hook form's native coercion. When we first introduced coercion via the react context setup we've got now, we were using react hook form v5 which had no native answer. In v6 they added a few options, like valueAsNumber, valueAsDate, and setValueAs (which lets you provide a function to handle the rest of the cases). You can find documentation for them under register here. Would you be up for deprecating the coercion setup we've got going in favor of react hook form's options? Just let me know—I'm happy to merge this one and do it as a separate follow up PR! Thank you again for this. |
|
@jtoar why don't we give this a few more hours today. If Brian doesn't respond, we can assume he's heads down on other things for the time being. So merge this, then open a new issue and loop him in (regarding #3043 (comment)). Sound good? |
|
@jtoar @thedavidprice Sorry, I've been crazy busy whole this week coz I moved on and joined a new team this Monday. If possible, may I suggest that we merge first then revisit later? I don't think I will have time in the follow few days to tackle this, it would be good to give me few days to settle in my new team then I will pick it up later. |
|
@jtoar For coercion refactoring, would you mind create an issue and assign to me so I can follow up later? appreciate 😄 |
|
CI error: What the what??? |
|
@thedavidprice I ran it locally to see if I could reproduce the error but no luck; thanks for rerunning! |
|
It almost feels like it's not starting from a new directory... like it's building on top of something existing. I'll take a look. |
…om-functions-test * 'main' of github.com:redwoodjs/redwood: Better SEO by default on every pages (redwoodjs#3026) Add more info in the terminal | Tweak gitpod settings (redwoodjs#3185) Undo accidentally vscode settings change (redwoodjs#3184) Upgrade RHF to v7.11.0 (redwoodjs#3043) [setup tailwind] Include tailwind by adding directives to index.css (redwoodjs#3181) Configure cloud based development for contributors (redwoodjs#3150)
This is the same task as #2270 to upgrade
react-hook-formto v7 (latestv7.11.0), I created a separated PR coz I don't wanna contaminate the original PR.@morganmspencer I hope you don't mind that I took the liberty to create a separated PR for the same work you already started mainly try not to contaminate your great works so far. I was tagged by @thedavidprice and @Tobbe a week ago while I was fixing a
formrelated bug #2979 see if I could potentially jump in to help on this. I am happy for you to cherry pick my changes into your PR if you are willing to as you should take the credit as your PR actually speed up my work a lot.Anyway, even unit tests are all passed and tested with my application, Im still not 100% confident, so would be good to have more eyes on this. Also, open for naming suggestion in general if current naming doesn't provide clear context.
Btw, I noticed a weird behaviour while testing with my application, the
formState.dirtyFieldsreturnsemptythe first time when you change a field then blur, it seems related to this issue happened inv7.0.7.formState.touchedFieldsandformState.isDirtyare all working fine, I don't think this is related toredwooddirectly, but worth to mention.