-
-
Notifications
You must be signed in to change notification settings - Fork 597
fix(form-core): establish Field-over-Form prioritization for isDefaultValue and resets #2006
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 4 commits
4567071
75dbab5
9dead10
53da788
699134c
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,5 @@ | ||
| --- | ||
| '@tanstack/form-core': minor | ||
| --- | ||
|
|
||
| Introduced a **Prioritized Default System** that ensures consistency between field metadata and form reset behavior. This change prioritizes field-level default values over form-level defaults across `isDefaultValue` derivation, `form.reset()`, and `form.resetField()`. This ensures that field metadata accurately reflects the state the form would return to upon reset and prevents `undefined` from being incorrectly treated as a default when a value is explicitly specified. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -120,7 +120,7 @@ describe('field api', () => { | |
| expect(field.getMeta().isDefaultValue).toBe(false) | ||
|
|
||
| field.setValue('test') | ||
| expect(field.getMeta().isDefaultValue).toBe(true) | ||
| expect(field.getMeta().isDefaultValue).toBe(false) | ||
|
Contributor
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. Reminder to self: Check git blame. I don't this change is good, but I want to know the context of why it was explicitly listed as unit test.
Contributor
Author
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. @LeCarbonator Thanks for taking the time to review this! I checked the git blame - this test was intentionally written this way in PR #1456 It wasn't a mistake, so I want to be careful here. But I think there's a philosophical question: What should "default" mean when form-level and field-level disagree? The Two InterpretationsOriginal (OR logic): "Both are defaults" Proposed (?? logic): "The one Why I Lean Toward the Proposed LogicIn #1081, explained the purpose of
RHF's With the original OR logic:
That feels inconsistent with RHF's model. A form should have one default state, not two. The Practical Problem// User checks before reset
if (!field.getMeta().isDefaultValue) {
form.reset() // "I'm not at default, let me reset"
}With OR logic, if value is
The user gets misleading information. I Could Be WrongI understand the original design might have had reasons I'm not aware of. Maybe there are use cases where treating both as "default" makes sense. What's your take on this? |
||
|
|
||
| form.resetField('name') | ||
| expect(field.getMeta().isDefaultValue).toBe(true) | ||
|
|
@@ -130,6 +130,54 @@ describe('field api', () => { | |
| expect(field.getMeta().isDefaultValue).toBe(true) | ||
| }) | ||
|
|
||
| it('should be false when value is undefined and a default value is specified in form-level only', () => { | ||
| const form = new FormApi({ | ||
| defaultValues: { | ||
| name: 'foo', | ||
| }, | ||
| }) | ||
| form.mount() | ||
|
|
||
| const field = new FieldApi({ | ||
| form, | ||
| name: 'name', | ||
| }) | ||
| field.mount() | ||
|
|
||
| expect(field.getMeta().isDefaultValue).toBe(true) | ||
|
|
||
| // Set to undefined - should be false because 'foo' is the default | ||
| field.setValue(undefined as any) | ||
| expect(field.getMeta().isDefaultValue).toBe(false) | ||
| }) | ||
|
|
||
| it('should handle falsy values correctly in isDefaultValue', () => { | ||
| const form = new FormApi({ | ||
| defaultValues: { | ||
| count: 0, | ||
| active: false, | ||
| text: '', | ||
| }, | ||
| }) | ||
| form.mount() | ||
|
|
||
| const countField = new FieldApi({ form, name: 'count' }) | ||
| const activeField = new FieldApi({ form, name: 'active' }) | ||
| const textField = new FieldApi({ form, name: 'text' }) | ||
| countField.mount() | ||
| activeField.mount() | ||
| textField.mount() | ||
|
|
||
| expect(countField.getMeta().isDefaultValue).toBe(true) | ||
| expect(activeField.getMeta().isDefaultValue).toBe(true) | ||
| expect(textField.getMeta().isDefaultValue).toBe(true) | ||
|
|
||
| countField.setValue(1) | ||
| expect(countField.getMeta().isDefaultValue).toBe(false) | ||
| countField.setValue(0) | ||
| expect(countField.getMeta().isDefaultValue).toBe(true) | ||
| }) | ||
|
|
||
| it('should update the fields meta isDefaultValue with arrays - simple', () => { | ||
| const form = new FormApi({ | ||
| defaultValues: { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.