Fix FormApi.submit return type#554
Conversation
📝 WalkthroughWalkthroughThe PR updates the form submission API to return ChangesSubmission Errors and useSyncExternalStore API Expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/FinalForm.ts`:
- Around line 1357-1362: Sync onSubmit return values are being dropped because
the sync branch calls complete(result as SubmissionErrors) but then falls
through to return Promise.resolve(undefined as SubmissionErrors); update the
sync branch in submit (the block handling onSubmit when it returns a
non-Promise) to forward the result of complete (or directly return
Promise.resolve(result as SubmissionErrors)) so the Promise returned by submit
resolves to the errors object; locate the sync path around the onSubmit handling
where complete is invoked and replace the fall-through with a return that
carries the result through.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a7f7a9b1-3d17-40e2-9523-17a56016d602
📒 Files selected for processing (2)
src/FinalForm.tssrc/types.ts
| } else { | ||
| // onSubmit is sync | ||
| complete(result as FormValues); | ||
| complete(result as SubmissionErrors); | ||
| } | ||
| } | ||
| return Promise.resolve(undefined as FormValues | undefined); | ||
| return Promise.resolve(undefined as SubmissionErrors); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find tests that combine sync onSubmit returning errors with awaited submit().
rg -nP -C5 '\bonSubmit\s*[:=]\s*(\([^)]*\)\s*=>\s*\(?\s*\{|jest\.fn\(\(\)\s*=>\s*\(?\s*\{)' --type=ts -g '**/*test*'
echo '---'
# Show all sites that await/return form.submit() and any nearby expect on the resolved value.
rg -nP -C4 '\b(await|return)\s+form\.submit\s*\(' --type=tsRepository: final-form/final-form
Length of output: 19189
🏁 Script executed:
sed -n '355,385p' src/FinalForm.submission.test.tsRepository: final-form/final-form
Length of output: 971
🏁 Script executed:
sed -n '335,390p' src/FinalForm.submission.test.tsRepository: final-form/final-form
Length of output: 1658
🏁 Script executed:
sed -n '410,450p' src/FinalForm.submission.test.tsRepository: final-form/final-form
Length of output: 1524
🏁 Script executed:
# Search for tests that check the result of form.submit() and look for any with sync onSubmit
rg -n -B10 -A5 'const result = await form\.submit\(\)' src/FinalForm.submission.test.ts | head -100Repository: final-form/final-form
Length of output: 618
🏁 Script executed:
sed -n '1340,1365p' src/FinalForm.tsRepository: final-form/final-form
Length of output: 903
🏁 Script executed:
# Find the complete function definition within the submit function
sed -n '1200,1365p' src/FinalForm.ts | grep -A20 'const complete'Repository: final-form/final-form
Length of output: 838
🏁 Script executed:
# Get the full complete function definition
sed -n '1210,1240p' src/FinalForm.tsRepository: final-form/final-form
Length of output: 1086
🏁 Script executed:
# Search for the complete function definition
rg -n 'const complete = \(' src/FinalForm.ts -A25Repository: final-form/final-form
Length of output: 1096
🏁 Script executed:
# Look for all places where form.submit() is awaited and check onSubmit definitions nearby
rg -n -B30 'const result = await form\.submit\(\)|await form\.submit\(\)' src/FinalForm.submission.test.ts | grep -E '(onSubmit|const result|await form\.submit)' | head -40Repository: final-form/final-form
Length of output: 1733
🏁 Script executed:
# Look at the test where line 375 has "const result = await form.submit()"
sed -n '340,380p' src/FinalForm.submission.test.tsRepository: final-form/final-form
Length of output: 1333
🏁 Script executed:
# Check if there's a test specifically for sync onSubmit returning errors with awaited result
rg -n -B5 'onSubmit.*=>.*\{' src/FinalForm.submission.test.ts | grep -A10 'onSubmit: .*=>.*{$' | head -60Repository: final-form/final-form
Length of output: 727
🏁 Script executed:
# Check the test around line 637 that has sync onSubmit returning errors
sed -n '632,680p' src/FinalForm.submission.test.tsRepository: final-form/final-form
Length of output: 1415
🏁 Script executed:
# Look at the three paths in the submit function more carefully
sed -n '1330,1365p' src/FinalForm.tsRepository: final-form/final-form
Length of output: 1325
🏁 Script executed:
# Search for all tests that await form.submit() to find which ones test sync vs async onSubmit
rg -n -B20 'const result = await form\.submit\(\)' src/FinalForm.submission.test.ts | grep -E '(it\(|onSubmit.*=|const result)' | head -20Repository: final-form/final-form
Length of output: 109
🏁 Script executed:
# Check if there are any tests that might be new/modified that test sync onSubmit with awaited result
rg -n 'sync.*onSubmit|onSubmit.*sync' src/FinalForm.submission.test.tsRepository: final-form/final-form
Length of output: 829
🏁 Script executed:
# Check if there are any new tests added in this PR that test sync onSubmit with awaited result
git log --oneline -20Repository: final-form/final-form
Length of output: 105
Sync onSubmit returning errors is silently dropped from the resolved Promise.
The asymmetry between the three submit paths breaks the new return-type contract:
- Async (Promise) path at L1340-1349: returns
valuefrom the.thenhandler → caller gets the errors. - Callback path at L1354-1356:
resolvePromise(errors)is invoked fromcomplete→ caller gets the errors. - Sync path at L1357-1361:
complete(result as SubmissionErrors)is called for its side-effects only; its return value (the errors) is discarded, and execution falls through toreturn Promise.resolve(undefined as SubmissionErrors)at L1362.
For a synchronous onSubmit returning an errors object, await form.submit() will resolve to undefined instead of the expected errors object. This path is not covered by the existing test suite.
🛠️ Suggested fix
Forward the result of complete (or short-circuit the fall-through) so the resolved Promise carries the errors from a sync onSubmit:
} else {
// onSubmit is sync
- complete(result as SubmissionErrors);
+ return Promise.resolve(complete(result as SubmissionErrors));
}
}
return Promise.resolve(undefined as SubmissionErrors);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else { | |
| // onSubmit is sync | |
| complete(result as FormValues); | |
| complete(result as SubmissionErrors); | |
| } | |
| } | |
| return Promise.resolve(undefined as FormValues | undefined); | |
| return Promise.resolve(undefined as SubmissionErrors); | |
| } else { | |
| // onSubmit is sync | |
| return Promise.resolve(complete(result as SubmissionErrors)); | |
| } | |
| } | |
| return Promise.resolve(undefined as SubmissionErrors); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/FinalForm.ts` around lines 1357 - 1362, Sync onSubmit return values are
being dropped because the sync branch calls complete(result as SubmissionErrors)
but then falls through to return Promise.resolve(undefined as SubmissionErrors);
update the sync branch in submit (the block handling onSubmit when it returns a
non-Promise) to forward the result of complete (or directly return
Promise.resolve(result as SubmissionErrors)) so the Promise returned by submit
resolves to the errors object; locate the sync path around the onSubmit handling
where complete is invoked and replace the fall-through with a return that
carries the result through.
1 participant
Closes #368.\n\n## Summary\n- updates FormApi.submit to return SubmissionErrors instead of FormValues\n- aligns createForm's submit implementation annotations/casts with onSubmit's documented return value\n\n## Verification\n- yarn test\n- yarn nps typescript\n- yarn nps build
Summary by CodeRabbit