fix(react): form.reset not working inside of onSubmit #1494
Conversation
|
View your CI Pipeline Execution ↗ for commit 840aaf9.
☁️ Nx Cloud last updated this comment at |
harry-whorlow
changed the title
|
I'm not sure if this is the proper fix, but this fixes the newly added test without introducing other test failures. Test is failing given |
|
@bpinto thanks for taking a look into this, I was rather busy over the weekend, so it's super appreciated! hmm thats, interesting... I'll shoot crutch corn a message since he authored the code, but you are correct it dose pass the tests, though to play devils advocate we are lacking framework specific tests so it might be breaking something. 😊 Once again thanks for the investigation! [edit] So to me removing the update(ops) won't work as we need to update the opts passed to form if they change. It's really a question as to what is causing the re-render of the form. |
|
these lines changing Of course it breaks everything else, but perhaps this could help with resolving this. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1494 +/- ##
==========================================
+ Coverage 89.13% 89.17% +0.03%
==========================================
Files 31 31
Lines 1417 1422 +5
Branches 362 364 +2
==========================================
+ Hits 1263 1268 +5
Misses 137 137
Partials 17 17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Testing on stackblitz, this appears to work as expected in react now! |
| @@ -216,7 +225,7 @@ export function useForm< | |||
| */ | |||
| useIsomorphicLayoutEffect(() => { | |||
| formApi.update(opts) | |||
| }) | |||
| }, [stableOptsRef.current]) | |||
There was a problem hiding this comment.
Given this value (stableOptsRef.current) is calculated on every re-render, I don't think using it as a [deps] adds any additional benefit over doing the check inside of the useIsomorphicLayoutEffect function.
useIsomorphicLayoutEffect(() => {
if (!evaluate(opts, stableOptsRef.current) {
formApi.update(opts)
}
})The reason for why I bring this up, is because I personally would rather avoid using a toString() to compare functions inside the evaluate function as that seems fragile/expensive. Therefore, there might be some alternative checks we could do here.
I don't know what the shouldUpdateReeval inside formApi.update() function is for but for the other 2 updates, if the form has been touched (!this.state.isTouched) then the update is skipped. So maybe there is something we can check to avoid the update() call altogether and potentially not need to deal with function toString() comparison.
useIsomorphicLayoutEffect(() => {
if (!formApi.form.state.isTouched) {
formApi.update(opts)
}
})There was a problem hiding this comment.
hmm... My concern here is that this issue only exists in the react sphere, all the other frameworks are working correctly, and modify core just to service a bug that only exists in react feels off.
I think I'll shelve this for a short minuet to look into other solutions, I appreciate the input.
Relates to #1490, #1485 and potentially #1487.
This appears to stem from the react adapter as Angular, Vue and Core all seem to work as intended.
Reset will work correctly initially then on next pass wipe the previous state. Curiously this seems to only be the case in the onSubmit handler.