fix(react): form.reset not working inside of onSubmit #1494
Conversation
|
View your CI Pipeline Execution ↗ for commit 6942440.
☁️ 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.24% 89.28% +0.03%
==========================================
Files 31 31
Lines 1432 1437 +5
Branches 366 368 +2
==========================================
+ Hits 1278 1283 +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! |
packages/react-form/src/useForm.tsx
Outdated
| @@ -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.
|
@harry-whorlow I tested out this PR, and I see a UI glitch whenever I reset the form with the new values from the API. The initial values are briefly displayed and then replaced with the new values. |
From discord correct? this was unrelated if I'm not mistaken. 🤘 |
|
So this is not an ideal solution, and we've talked about this internally, however we couldn’t come up with an alternative solution. I'll leave this up till Friday, so if anyone has any suggestions or other concerns they can post them here and I'll investigate. Otherwise I'll merge 🙂 |
|
I am not sure what type of API is meant to be implemented here so I wrote a failing tests below that may or may not be failing correctly. On this test, one of the form options This is an alternative way of introducing the bug that this PR is trying to fix as the diff --git a/packages/react-form/tests/useForm.test.tsx b/packages/react-form/tests/useForm.test.tsx
index 7b012db7..d042c095 100644
--- a/packages/react-form/tests/useForm.test.tsx
+++ b/packages/react-form/tests/useForm.test.tsx
@@ -797,7 +797,10 @@ describe('useForm', () => {
it('form should reset default value when resetting in onSubmit', async () => {
function Comp() {
+ const [asyncDebounceMs, setAsyncDebounceMs] = useState(0)
+
const form = useForm({
+ asyncDebounceMs,
defaultValues: {
name: '',
},
@@ -835,6 +838,10 @@ describe('useForm', () => {
<button type="reset" data-testid="reset" onClick={() => form.reset()}>
Reset
</button>
+
+ <button type="button" data-testid="update-form-options" onClick={() => setAsyncDebounceMs(10)}>
+ Update form options
+ </button>
</form>
)
}
@@ -843,6 +850,7 @@ describe('useForm', () => {
const input = getByTestId('fieldinput')
const submit = getByTestId('submit')
const reset = getByTestId('reset')
+ const updateFormOptions = getByTestId('update-form-options')
await user.type(input, 'test')
await waitFor(() => expect(input).toHaveValue('test'))
@@ -853,6 +861,9 @@ describe('useForm', () => {
await user.type(input, 'another-test')
await user.click(submit)
await waitFor(() => expect(input).toHaveValue('another-test'))
+
+ await user.click(updateFormOptions)
+ await waitFor(() => expect(input).toHaveValue('another-test'))
})
it('form should update when props are changed', async () => {A smaller patch simulating this issue: diff --git a/packages/react-form/tests/useForm.test.tsx b/packages/react-form/tests/useForm.test.tsx
index 7b012db7..d129ad8e 100644
--- a/packages/react-form/tests/useForm.test.tsx
+++ b/packages/react-form/tests/useForm.test.tsx
@@ -797,7 +797,10 @@ describe('useForm', () => {
it('form should reset default value when resetting in onSubmit', async () => {
function Comp() {
+ const [asyncDebounceMs, setAsyncDebounceMs] = useState(0)
+
const form = useForm({
+ asyncDebounceMs,
defaultValues: {
name: '',
},
@@ -805,6 +808,7 @@ describe('useForm', () => {
expect(value).toEqual({ name: 'another-test' })
form.reset(value)
+ setAsyncDebounceMs(10)
},
})As far as I understand, the core of the issue isn't being addressed on this PR yet since invoking I believe the reason for this is to support async initial values however it seems that |
|
@bpinto, thanks for the input! 😄 Your first test is failing correctly though isn't it? Essentially your passing in the form's props when the asyncDebounceMs is changed: { asyncDebounceMs, defaultValues: { name: '' } }So expecting "name: another-value" would fail. I see your line of thinking though, but it doesn’t really match up with my model of how I expect state change. Correct me if I'm wrong, but you're suggesting that form should ignore any future changes made to the default values after a form.reset() is called? I'll run it by the other maintainers though, see what they think. 🙂 |
👍 Yep, exactly. I think that form should ignore any future changes made to default values after a However, I want to emphasize one thing that you are questioning to confirm we are talking about the same thing:
in the tests above, the |
I was running into this same issue. I was submitting the form, If it helps anyone, calling |
Recording.2025-06-16.153353.mp4with the new values.
Confirmed the same issue: |
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.