-
-
Notifications
You must be signed in to change notification settings - Fork 496
Fix #984: useField returns stale values when sibling updates form in useEffect #1085
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,55 @@ | ||||||||||||||
| import * as React from "react"; | ||||||||||||||
| import { render, waitFor } from "@testing-library/react"; | ||||||||||||||
| import "@testing-library/jest-dom"; | ||||||||||||||
| import Form from "./ReactFinalForm"; | ||||||||||||||
| import { useField } from "./index"; | ||||||||||||||
|
|
||||||||||||||
| const onSubmitMock = (_values) => {}; | ||||||||||||||
|
|
||||||||||||||
| describe("useField issue #984", () => { | ||||||||||||||
| // https://github.com/final-form/react-final-form/issues/984 | ||||||||||||||
| // When a parent component's useEffect changes a form value, | ||||||||||||||
| // sibling components' useField should receive the updated value. | ||||||||||||||
| it("should get newest value when sibling updates form in useEffect", async () => { | ||||||||||||||
| const Field1 = () => { | ||||||||||||||
| const { input } = useField("field1"); | ||||||||||||||
| return <input {...input} data-testid="field1" />; | ||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| const Field2 = () => { | ||||||||||||||
| const { input } = useField("field1", { subscription: { value: true } }); | ||||||||||||||
| // Should show "UpdatedByField1" after ParentWithEffect's useEffect runs | ||||||||||||||
| return <span data-testid="field1-value">{input.value}</span>; | ||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| const ParentWithEffect = () => { | ||||||||||||||
| const { input } = useField("field1"); | ||||||||||||||
| React.useEffect(() => { | ||||||||||||||
| // Simulate programmatic change during effect phase | ||||||||||||||
| input.onChange("UpdatedByField1"); | ||||||||||||||
| }, []); | ||||||||||||||
| return null; | ||||||||||||||
| }; | ||||||||||||||
|
|
||||||||||||||
| const { getByTestId } = render( | ||||||||||||||
| <Form | ||||||||||||||
| onSubmit={onSubmitMock} | ||||||||||||||
| initialValues={{ field1: "InitialField1" }} | ||||||||||||||
| > | ||||||||||||||
| {() => ( | ||||||||||||||
| <form> | ||||||||||||||
| <ParentWithEffect /> | ||||||||||||||
| <Field1 /> | ||||||||||||||
| <Field2 /> | ||||||||||||||
| </form> | ||||||||||||||
| )} | ||||||||||||||
| </Form> | ||||||||||||||
| ); | ||||||||||||||
|
|
||||||||||||||
| // After useEffect runs, Field2 should see the updated value | ||||||||||||||
| // This is the bug: Field2 sees stale "InitialField1" instead | ||||||||||||||
| await waitFor(() => { | ||||||||||||||
| expect(getByTestId("field1-value").textContent).toBe("UpdatedByField1"); | ||||||||||||||
| }); | ||||||||||||||
|
Comment on lines
+51
to
+53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Prefer Since ♻️ Proposed fix await waitFor(() => {
- expect(getByTestId("field1-value").textContent).toBe("UpdatedByField1");
+ expect(getByTestId("field1-value")).toHaveTextContent("UpdatedByField1");
});📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| }); | ||||||||||||||
| }); | ||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -126,13 +126,19 @@ function useField< | |
|
|
||
| return { | ||
| active: false, | ||
| blur: () => { }, | ||
| change: () => { }, | ||
| blur: () => { | ||
| form.blur(name as keyof FormValues); | ||
| }, | ||
| change: (value) => { | ||
| form.change(name as keyof FormValues, value); | ||
| }, | ||
| data: data || {}, | ||
| dirty: false, | ||
| dirtySinceLastSubmit: false, | ||
| error: undefined, | ||
| focus: () => { }, | ||
| focus: () => { | ||
| form.focus(name as keyof FormValues); | ||
| }, | ||
| initial: initialStateValue, | ||
| invalid: false, | ||
| length: undefined, | ||
|
|
@@ -184,6 +190,71 @@ function useField< | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [name, data, defaultValue, initialValue]); | ||
|
|
||
| // FIX #988: When initialValue prop changes, update the form's initialValues | ||
| // for this field. This ensures that when a parent component updates initialValues | ||
| // after a save operation, the field becomes pristine if the value matches. | ||
| const prevInitialValueRef = React.useRef(initialValue); | ||
| React.useEffect(() => { | ||
| // Only run when initialValue actually changes (not on mount) | ||
| if ( | ||
| prevInitialValueRef.current !== initialValue && | ||
| initialValue !== undefined | ||
| ) { | ||
| prevInitialValueRef.current = initialValue; | ||
|
|
||
| // Get current form state | ||
| const formState = form.getState(); | ||
| const currentFormInitial = formState.initialValues | ||
| ? getIn(formState.initialValues, name) | ||
| : undefined; | ||
|
|
||
| // Only update if the new initialValue differs from current form initial | ||
| if (initialValue !== currentFormInitial) { | ||
| const currentValue = getIn(formState.values, name); | ||
|
|
||
| // If the current value matches the new initial value, update the form's | ||
| // initialValues to reflect this. This is needed for radio buttons where | ||
| // the user changes the value, then the parent saves and passes back the | ||
| // new initial value that matches what the user selected. | ||
| // | ||
| // We need to manually update formState.initialValues and notify listeners. | ||
| // Final Form doesn't expose a public API for this, so we use internal state. | ||
| const fieldState = form.getFieldState(name as keyof FormValues); | ||
| if (fieldState) { | ||
| // Force an update through the field subscriber by triggering a change | ||
| // to the same value, which will recalculate dirty state with new initial | ||
| if (currentValue === initialValue) { | ||
| // The value matches the new initial, so field should become pristine. | ||
|
Comment on lines
+224
to
+227
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check if isEqual is used elsewhere in the codebase for value comparisons
rg -n "isEqual" --type=ts src/Repository: final-form/react-final-form Length of output: 413 🏁 Script executed: # Read the useField.ts file focusing on the effect at lines 193-256
sed -n '193,256p' src/useField.ts | cat -nRepository: final-form/react-final-form Length of output: 3622 🏁 Script executed: # Also check the config/types structure to understand isEqual availability
sed -n '1,100p' src/useField.ts | cat -nRepository: final-form/react-final-form Length of output: 3824 🏁 Script executed: # Check how isEqual is actually used in useWhenValueChanges for comparison patterns
sed -n '1,30p' src/useWhenValueChanges.ts | cat -nRepository: final-form/react-final-form Length of output: 525 🏁 Script executed: # Search for test cases using initialValue with objects/arrays or non-primitive values
rg -n "initialValue" --type=ts --type=tsx -B2 -A2 src/__tests__/ | head -100Repository: final-form/react-final-form Length of output: 98 🏁 Script executed: # Check if there are examples or documentation about object/array initialValues
rg -n "initialValue.*\{|\[" --type=ts --type=tsx src/ | head -50Repository: final-form/react-final-form Length of output: 98 🏁 Script executed: # Check what the default isEqual should be when config.isEqual is undefined
rg -n "isEqual.*=" --type=ts -B1 -A1 src/types.tsRepository: final-form/react-final-form Length of output: 167 🏁 Script executed: # Search for how registerField is called to see how isEqual is handled
rg -n "registerField" src/useField.ts -B2 -A10 | head -60Repository: final-form/react-final-form Length of output: 1943 🏁 Script executed: # Look for actual usage of initialValue with objects/arrays in tests
find src -name "*.test.ts" -o -name "*.test.js" | xargs rg -l "initialValue" 2>/dev/null | head -5Repository: final-form/react-final-form Length of output: 207 🏁 Script executed: # Check final-form package to see how it handles isEqual (look at node_modules or imports)
rg -n "isEqual" src/useField.ts -B3 -A3Repository: final-form/react-final-form Length of output: 271 🏁 Script executed: # Look at the test file for useField to see if there are object/array initialValue tests
head -200 src/useField.test.js | cat -nRepository: final-form/react-final-form Length of output: 8734 🏁 Script executed: # Search specifically for initialValue tests with objects
rg -n "initialValue.*\{" src/useField.test.js -A5 | head -80Repository: final-form/react-final-form Length of output: 2529 🏁 Script executed: # Check the issue-984 test since the comment mentions FIX `#988`
cat src/useField.issue-984.test.js | head -100Repository: final-form/react-final-form Length of output: 1837 The strict equality comparison may fail for object/array Both the condition at line 8 ( Use 🤖 Prompt for AI Agents |
||
| // Re-register with new initialValue to update formState.initialValues. | ||
| // Final Form's registerField will update initialValues when: | ||
| // - value === old initial (meaning pristine before) | ||
| // We need to handle the case where value === new initial but value !== old initial | ||
| // | ||
| // Workaround: We need to update formState.initialValues directly. | ||
| // The only public API is form.setConfig('initialValues', ...) but that | ||
| // resets ALL values. Instead, we use a workaround: | ||
| // Trigger a re-registration which will update initialValues for this field. | ||
| form.pauseValidation(); | ||
| try { | ||
| // Manually update initialValues via registerField with silent: false | ||
| // to force notification | ||
| const unsubscribe = form.registerField( | ||
| name as keyof FormValues, | ||
| () => {}, | ||
| {}, | ||
| { initialValue } | ||
| ); | ||
| // Immediately unsubscribe to avoid orphan subscriber | ||
| unsubscribe(); | ||
| } finally { | ||
| form.resumeValidation(); | ||
| } | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
| } | ||
| } | ||
| }, [initialValue, name, form]); | ||
|
|
||
| const meta: any = {}; | ||
| addLazyFieldMetaState(meta, state); | ||
| const getInputValue = () => { | ||
|
|
@@ -245,7 +316,7 @@ function useField< | |
| const input: FieldInputProps<FieldValue, T> = { | ||
| name, | ||
| onBlur: useConstantCallback((_event?: React.FocusEvent<any>) => { | ||
| state.blur(); | ||
| form.blur(name as keyof FormValues); | ||
| if (formatOnBlur) { | ||
| /** | ||
| * Here we must fetch the value directly from Final Form because we cannot | ||
|
|
@@ -254,9 +325,9 @@ function useField< | |
| * before calling `onBlur()`, but before the field has had a chance to receive | ||
| * the value update from Final Form. | ||
| */ | ||
| const fieldState = form.getFieldState(state.name as keyof FormValues); | ||
| const fieldState = form.getFieldState(name as keyof FormValues); | ||
| if (fieldState) { | ||
| state.change(format(fieldState.value, state.name)); | ||
| form.change(name as keyof FormValues, format(fieldState.value, name)); | ||
| } | ||
| } | ||
| }), | ||
|
|
@@ -282,14 +353,16 @@ function useField< | |
| } | ||
| } | ||
|
|
||
| const currentValue = | ||
| form.getFieldState(name as keyof FormValues)?.value ?? state.value; | ||
| const value: any = | ||
| event && event.target | ||
| ? getValue(event, state.value, _value, isReactNative) | ||
| ? getValue(event, currentValue, _value, isReactNative) | ||
| : event; | ||
| state.change(parse(value, name)); | ||
| form.change(name as keyof FormValues, parse(value, name)); | ||
| }), | ||
| onFocus: useConstantCallback((_event?: React.FocusEvent<any>) => | ||
| state.focus(), | ||
| form.focus(name as keyof FormValues), | ||
| ), | ||
| get value() { | ||
| return getInputValue(); | ||
|
|
||
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.
🧹 Nitpick | 🔵 Trivial
Consider adding ESLint disable comment for exhaustive-deps.
The empty dependency array is intentional to simulate a mount-only effect. Since
input.onChangeis stable (viauseConstantCallback), this is safe, but ESLint'sreact-hooks/exhaustive-depsrule would flag it. Adding a disable comment documents the intent.♻️ Proposed fix
React.useEffect(() => { // Simulate programmatic change during effect phase input.onChange("UpdatedByField1"); + // eslint-disable-next-line react-hooks/exhaustive-deps }, []);📝 Committable suggestion
🤖 Prompt for AI Agents