fix(useField): Fix no re-render on initialValue change#1034
fix(useField): Fix no re-render on initialValue change#1034jspannu919 wants to merge 1 commit intofinal-form:mainfrom
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit fe2fb36:
|
|
IMO this is fine, you should not listen for |
|
@bslipek In data driven forms (Ref: https://github.com/data-driven-forms/react-forms ) for conditionally hiding the fields, the fields are wrapped with a Field component so that they get re-rendered when the value in any of the parent attribute change(Ref: https://github.com/data-driven-forms/react-forms/blob/84c4cb8861d689cb87ba68100fe7d3acfd667655/packages/react-form-renderer/src/form-renderer/render-form.js#L47). So, due to the above issue the conditional visibility doesn't works if we reverse the order of the fields(Ref: https://stackblitz.com/edit/1dkk8s-rd1ejn?file=schema.js). |
|
I got your point. But still think that this is not final-form problem. But I am just a observer here :) |
|
Everything is in the name of the props "initialValue" |
|
Thanks for this PR! There's an interesting design question here. The issue: Community perspective (from comments): Code concerns:
Design question for @erikras:
My instinct: The name "initialValue" suggests it's for initialization, not dynamic updates. For dynamic defaults, use form-level What's your intent here? 🤔 |
erikras-richard-agent
left a comment
erikras-richard-agent
left a comment
There was a problem hiding this comment.
Review by Richard (Team Lead)
This addresses a real bug: when initialValue changes on first render, the field state doesn't update because the first render callback is skipped entirely.
Issues:
- Missing space:
if(state.initial != newState.initial)— needs space afterifand should use!==(strict equality) - Shallow comparison:
state.initial != newState.initialwon't work for object/array initial values. Should use deep equality or at minimumJSON.stringifycomparison. - Test is incomplete: The test name is
should listen to initial value 2(not descriptive), theexpect(input.value).toBe('test')is commented out, and there's no assertion that the value actually propagates correctly. - Variable shadowing: The outer
statefrom the closure andnewStateparameter could be confusing. The original code reusedstateas the param name intentionally.
Verdict: Right direction but needs polish. The bug is real — would be better to fix this properly in our codebase with strict equality and proper test coverage.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR fixes initial value propagation in the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/useField.js`:
- Around line 115-123: The register callback currently closes over state and
uses loose inequality (state.initial != newState.initial); change it to avoid
reading the captured state by using the setState functional updater and compare
newState.initial to the previous state's initial using strict !==; specifically,
inside the register((newState) => { ... }) callback, call setState(prev => { if
(firstRender.current) { firstRender.current = false; if (prev.initial !==
newState.initial) return newState; return prev; } return newState; }) so you
never reference the outer `state` variable and you use strict comparison for
`initial`.
In `@src/useField.test.js`:
- Around line 459-476: The test currently only asserts meta.initial inside
MyFieldListener on the second render, so it will pass if no re-render occurs;
update the test to explicitly fail when the re-render never happens by adding a
wait/assertion after render that checks meta.initial becomes "test" (e.g., use
waitFor or a polling expect) or have MyFieldListener flip a ref/state or call a
jest mock when it observes meta.initial === "test" and then assert that mock was
called; target the MyFieldListener component and the useField hook's
meta.initial to ensure the test fails if the re-render never occurs.
| register((newState) => { | ||
| if (firstRender.current) { | ||
| firstRender.current = false; | ||
| if(state.initial != newState.initial){ | ||
| setState(newState); | ||
| } | ||
| } else { | ||
| setState(state); | ||
| setState(newState); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "useField.js" | head -20Repository: final-form/react-final-form
Length of output: 88
🏁 Script executed:
cat -n ./src/useField.js | head -150Repository: final-form/react-final-form
Length of output: 5439
🏁 Script executed:
cat -n ./src/useField.js | sed -n '95,140p'Repository: final-form/react-final-form
Length of output: 1818
🏁 Script executed:
rg "type FieldState" --type js --type ts -A 10Repository: final-form/react-final-form
Length of output: 53
🏁 Script executed:
cat -n ./src/types.js | head -100Repository: final-form/react-final-form
Length of output: 117
🏁 Script executed:
fd "types" --type f -e ts -e js | head -20Repository: final-form/react-final-form
Length of output: 53
🏁 Script executed:
rg "registerField" --type js --type ts -B 2 -A 5 | head -50Repository: final-form/react-final-form
Length of output: 986
🏁 Script executed:
rg "form.registerField" -B 5 -A 15 | head -80Repository: final-form/react-final-form
Length of output: 1205
🏁 Script executed:
rg "Object.is\|defaultIsEqual" --type js -B 2 -A 2Repository: final-form/react-final-form
Length of output: 53
🏁 Script executed:
rg "isEqual" --type js -B 2 -A 2 | head -40Repository: final-form/react-final-form
Length of output: 2028
🏁 Script executed:
rg "first render\|firstRender" --type js -B 3 -A 3Repository: final-form/react-final-form
Length of output: 53
🏁 Script executed:
cat -n ./src/useField.js | sed -n '58,92p'Repository: final-form/react-final-form
Length of output: 1482
🏁 Script executed:
rg "FieldState" --type js -A 5 | head -40Repository: final-form/react-final-form
Length of output: 2017
🏁 Script executed:
cat -n ./src/useField.js | sed -n '113,130p'Repository: final-form/react-final-form
Length of output: 690
Violation of stale closure pattern: avoid capturing state in the register callback, and use strict equality.
Lines 59–63 explicitly warn against using state inside register closures because it captures stale state from the current execution context. Line 118 directly violates this by reading state.initial inside the callback. Additionally, loose equality (!=) can coerce null and undefined as equal, skipping real initial value changes. Use a functional updater with strict comparison instead.
🔧 Proposed fix
register((newState) => {
if (firstRender.current) {
firstRender.current = false;
- if(state.initial != newState.initial){
- setState(newState);
- }
+ setState((prev) =>
+ Object.is(prev.initial, newState.initial) ? prev : newState,
+ );
} else {
setState(newState);
}
}, false),🤖 Prompt for AI Agents
In `@src/useField.js` around lines 115 - 123, The register callback currently
closes over state and uses loose inequality (state.initial != newState.initial);
change it to avoid reading the captured state by using the setState functional
updater and compare newState.initial to the previous state's initial using
strict !==; specifically, inside the register((newState) => { ... }) callback,
call setState(prev => { if (firstRender.current) { firstRender.current = false;
if (prev.initial !== newState.initial) return newState; return prev; } return
newState; }) so you never reference the outer `state` variable and you use
strict comparison for `initial`.
| it("should listen to initial value 2", () => { | ||
| const MyFieldListener = () => { | ||
| const isFirstRender = React.useRef(true) | ||
| const { input, meta } = useField("name"); | ||
| if(!isFirstRender.current){ | ||
| expect(meta.initial).toBe("test"); | ||
| // expect(input.value).toBe("test"); | ||
| } | ||
| isFirstRender.current = false | ||
| return null; | ||
| }; | ||
| render( | ||
| <Form onSubmit={onSubmitMock}> | ||
| {() => ( | ||
| <form> | ||
| <MyFieldListener /> | ||
| <Field name="name" component="input" data-testid="name" initialValue="test"/> | ||
| </form> |
There was a problem hiding this comment.
Make the test fail if the re-render never happens.
The expectation runs only on the second render; if no re-render occurs, the test passes without exercising the behavior. Add an explicit wait/assertion that meta.initial becomes "test".
✅ Suggested adjustment
-import { render, fireEvent, cleanup } from "@testing-library/react";
+import { render, fireEvent, cleanup, waitFor } from "@testing-library/react";
- it("should listen to initial value 2", () => {
+ it("should listen to initial value 2", async () => {
+ const initialSpy = jest.fn();
const MyFieldListener = () => {
- const isFirstRender = React.useRef(true)
- const { input, meta } = useField("name");
- if(!isFirstRender.current){
- expect(meta.initial).toBe("test");
- // expect(input.value).toBe("test");
- }
- isFirstRender.current = false
+ const { meta } = useField("name");
+ React.useEffect(() => {
+ initialSpy(meta.initial);
+ }, [meta.initial]);
return null;
};
render(
<Form onSubmit={onSubmitMock}>
{() => (
<form>
<MyFieldListener />
<Field name="name" component="input" data-testid="name" initialValue="test"/>
</form>
)}
</Form>,
);
+ await waitFor(() => expect(initialSpy).toHaveBeenCalledWith("test"));
});🤖 Prompt for AI Agents
In `@src/useField.test.js` around lines 459 - 476, The test currently only asserts
meta.initial inside MyFieldListener on the second render, so it will pass if no
re-render occurs; update the test to explicitly fail when the re-render never
happens by adding a wait/assertion after render that checks meta.initial becomes
"test" (e.g., use waitFor or a polling expect) or have MyFieldListener flip a
ref/state or call a jest mock when it observes meta.initial === "test" and then
assert that mock was called; target the MyFieldListener component and the
useField hook's meta.initial to ensure the test fails if the re-render never
occurs.
|
Thank you for identifying a real issue with initialValue change detection in useField, @jspannu919! This PR targets the old JS codebase which has since been rewritten in TypeScript, so it's no longer directly mergeable. We'll evaluate whether the underlying issue still exists in the current code. Closing with appreciation. 🙏 |
|
Update: Verified that this bug is fixed in the current TypeScript codebase. The |
4 participants
Currently useField doesn't retriggers properly if the initialValue is changed (Added test scenario).
Added explicit check for initial value change to prevent deepEqual overheads.
Summary by CodeRabbit
Bug Fixes
Tests