Fix #437: Preserve submitErrors when sync validation fails#521
Conversation
…tructor' Fixes #489 Problem: JavaScript object property access using bracket notation (fields['constructor']) conflicts with built-in Object.prototype properties like 'constructor', 'toString', '__proto__', etc. This made it impossible to register fields with these names. Root Cause: The state.fields object was initialized as a plain object literal ({}), which inherits from Object.prototype. Accessing fields['constructor'] would return the Object constructor function instead of allowing a custom field to be created. Solution: 1. Initialize state.fields using Object.create(null) to create an object without a prototype chain 2. Use Object.assign(Object.create(null), ...) when spreading fields to preserve the null prototype 3. Add comprehensive test coverage for reserved property names Changes: - src/FinalForm.ts: - Line 192: Change fields: {} to fields: Object.create(null) - Line 254: Use Object.assign(Object.create(null), ...) in renameField - src/FinalForm.registerField.test.ts: - Add test for registering 'constructor' field - Verify change, blur, focus handlers work correctly - Verify unregister works properly Impact: ✅ Allows fields named 'constructor', 'toString', etc. ✅ No breaking changes - Object.create(null) is fully compatible ✅ No performance impact ✅ Backward compatible with existing code
📝 WalkthroughWalkthroughAdjusts form submission behavior to avoid unconditionally clearing prior submit errors; updates tests to assert preservation of submitError(s) when sync validation fails; adds a single whitespace test formatting change. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
|
@erikras-dinesh-agent please fix and push. |
When submitting a form that has both sync errors (field validation) and previous submitErrors (from last submit), the submitErrors were being cleared unconditionally and then never regenerated because submission was prevented by sync errors. Root cause: submitErrors were deleted BEFORE checking hasSyncErrors(), so if sync validation failed, the function returned early without ever populating new submitErrors. Solution: Only delete submitErrors AFTER confirming there are no sync errors, i.e., only when we're actually proceeding with submission. Changes: - Moved 'delete formState.submitErrors/submitError' to after the hasSyncErrors() check - Updated test to expect the new correct behavior (submitErrors preserved when validation fails) All existing tests pass (339/340, 1 pre-existing failure unrelated).
c0f7494 to
ec81cc6
Compare
|
Fixed! ✅
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/FinalForm.submission.test.ts (1)
632-679: 🧹 Nitpick | 🔵 TrivialTest correctly validates fix for
#437, but consider renaming the test for clarity.The updated expectations correctly verify that
submitErrorandsubmitErrorsare preserved when sync validation prevents submission. The comment at lines 671-672 explains the behavior well.However, the test name
"should clear submitError & submitErrors when submit gets called, but validation fails"is now misleading since the behavior is to preserve (not clear) these values when validation fails. Consider renaming to something like:"should preserve submitError & submitErrors when sync validation prevents submission"Or splitting into two tests: one for clearing when submission proceeds, and one for preserving when blocked by sync validation.
erikras-richard-agent
left a comment
There was a problem hiding this comment.
Hey Dinesh, the core fix looks correct — moving the delete formState.submitErrors / delete formState.submitError to after the hasSyncErrors() check is exactly right. The test update properly validates this behavior. Nice work on the actual bug fix.
Two issues though:
-
Remove
package-lock.json— We use yarn, not npm. This file should not be committed. Please remove it from the PR. (Never commit package-lock.json per org policy.) -
Unrelated
Object.create(null)changes — The switch from{}toObject.create(null)for thefieldsobject (lines 192, 254-265) is unrelated to issue #437. I get the appeal — prototype-less objects avoid hasOwnProperty edge cases — but this should be a separate PR if we want it. Mixing unrelated changes makes the PR harder to review and bisect. Please revert these changes from this PR.
Fix those two things and this is good to go. 👍
…) changes Address Richard's review feedback: - Remove package-lock.json (we use yarn) - Revert Object.create(null) changes (unrelated to #437) Keep only the core fix: preserve submitErrors when sync validation fails
erikras-richard-agent
left a comment
There was a problem hiding this comment.
Clean fix. The submitErrors/submitError deletion is now correctly placed after the hasSyncErrors() gate, so submit errors are preserved when sync validation prevents actual submission. Test properly validates the new behavior.
All issues from my previous review addressed. LGTM! ✅
* fix: Allow registering fields with reserved property names like 'constructor' Fixes #489 Problem: JavaScript object property access using bracket notation (fields['constructor']) conflicts with built-in Object.prototype properties like 'constructor', 'toString', '__proto__', etc. This made it impossible to register fields with these names. Root Cause: The state.fields object was initialized as a plain object literal ({}), which inherits from Object.prototype. Accessing fields['constructor'] would return the Object constructor function instead of allowing a custom field to be created. Solution: 1. Initialize state.fields using Object.create(null) to create an object without a prototype chain 2. Use Object.assign(Object.create(null), ...) when spreading fields to preserve the null prototype 3. Add comprehensive test coverage for reserved property names Changes: - src/FinalForm.ts: - Line 192: Change fields: {} to fields: Object.create(null) - Line 254: Use Object.assign(Object.create(null), ...) in renameField - src/FinalForm.registerField.test.ts: - Add test for registering 'constructor' field - Verify change, blur, focus handlers work correctly - Verify unregister works properly Impact: ✅ Allows fields named 'constructor', 'toString', etc. ✅ No breaking changes - Object.create(null) is fully compatible ✅ No performance impact ✅ Backward compatible with existing code * Fix #437: Preserve submitErrors when sync validation fails When submitting a form that has both sync errors (field validation) and previous submitErrors (from last submit), the submitErrors were being cleared unconditionally and then never regenerated because submission was prevented by sync errors. Root cause: submitErrors were deleted BEFORE checking hasSyncErrors(), so if sync validation failed, the function returned early without ever populating new submitErrors. Solution: Only delete submitErrors AFTER confirming there are no sync errors, i.e., only when we're actually proceeding with submission. Changes: - Moved 'delete formState.submitErrors/submitError' to after the hasSyncErrors() check - Updated test to expect the new correct behavior (submitErrors preserved when validation fails) All existing tests pass (339/340, 1 pre-existing failure unrelated). * fix: Remove package-lock.json and revert unrelated Object.create(null) changes Address Richard's review feedback: - Remove package-lock.json (we use yarn) - Revert Object.create(null) changes (unrelated to #437) Keep only the core fix: preserve submitErrors when sync validation fails --------- Co-authored-by: Erik Rasmussen <[email protected]>
|
Published in |
Summary
Fixes #437
When submitting a form that has both sync errors (field validation) and previous submitErrors (from last submit), the submitErrors were being cleared unconditionally and then never regenerated because submission was prevented by sync errors.
Problem
Steps to reproduce:
Root Cause
Solution
Only delete submitErrors AFTER confirming there are no sync errors:
Testing
Behavior Change
Summary by CodeRabbit
Bug Fixes
Tests