-
-
Notifications
You must be signed in to change notification settings - Fork 628
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
Changes from all commits
4567071
75dbab5
9dead10
53da788
699134c
51d5019
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 |
|---|---|---|
|
|
@@ -1132,16 +1132,12 @@ export class FormApi< | |
| // As primitives, we don't need to aggressively persist the same referential value for performance reasons | ||
| const isFieldValid = !isNonEmptyArray(fieldErrors) | ||
| const isFieldPristine = !currBaseMeta.isDirty | ||
| const isDefaultValue = | ||
| evaluate( | ||
| curFieldVal, | ||
| const isDefaultValue = evaluate( | ||
| curFieldVal, | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
| this.getFieldInfo(fieldName)?.instance?.options.defaultValue ?? | ||
| getBy(this.options.defaultValues, fieldName), | ||
| ) || | ||
| evaluate( | ||
| curFieldVal, | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
| this.getFieldInfo(fieldName)?.instance?.options.defaultValue, | ||
| ) | ||
| ) | ||
|
Comment on lines
+1135
to
+1140
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.
Using nullish coalescing here makes explicit Suggested fix-const isDefaultValue = evaluate(
- curFieldVal,
- this.getFieldInfo(fieldName)?.instance?.options.defaultValue ??
- getBy(this.options.defaultValues, fieldName),
-)
+const fieldDefaultValue =
+ this.getFieldInfo(fieldName).instance?.options.defaultValue
+const prioritizedDefaultValue =
+ fieldDefaultValue !== undefined
+ ? fieldDefaultValue
+ : getBy(this.options.defaultValues, fieldName)
+const isDefaultValue = evaluate(curFieldVal, prioritizedDefaultValue)-const fieldDefault =
- this.getFieldInfo(field).instance?.options.defaultValue
-const formDefault = getBy(this.options.defaultValues, field)
-const targetValue = fieldDefault ?? formDefault
+const fieldDefault =
+ this.getFieldInfo(field).instance?.options.defaultValue
+const formDefault = getBy(this.options.defaultValues, field)
+const targetValue = fieldDefault !== undefined ? fieldDefault : formDefaultAlso applies to: 2601-2605 🤖 Prompt for AI Agents |
||
|
|
||
| if ( | ||
| prevFieldInfo && | ||
|
|
@@ -1535,16 +1531,35 @@ export class FormApi< | |
| } | ||
| } | ||
|
|
||
| this.baseStore.setState(() => | ||
| getDefaultFormState({ | ||
| this.baseStore.setState(() => { | ||
| let nextValues = | ||
| values ?? | ||
| this.options.defaultValues ?? | ||
| this.options.defaultState?.values | ||
|
|
||
| if (!values) { | ||
| ;(Object.values(this.fieldInfo) as FieldInfo<any>[]).forEach( | ||
| (fieldInfo) => { | ||
| if ( | ||
| fieldInfo.instance && | ||
| fieldInfo.instance.options.defaultValue !== undefined | ||
| ) { | ||
| nextValues = setBy( | ||
| nextValues, | ||
| fieldInfo.instance.name, | ||
| fieldInfo.instance.options.defaultValue, | ||
| ) | ||
| } | ||
| }, | ||
| ) | ||
| } | ||
|
|
||
| return getDefaultFormState({ | ||
| ...(this.options.defaultState as any), | ||
| values: | ||
| values ?? | ||
| this.options.defaultValues ?? | ||
| this.options.defaultState?.values, | ||
| values: nextValues, | ||
| fieldMetaBase, | ||
| }), | ||
| ) | ||
| }) | ||
| }) | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -2583,15 +2598,21 @@ export class FormApi< | |
| */ | ||
| resetField = <TField extends DeepKeys<TFormData>>(field: TField) => { | ||
| this.baseStore.setState((prev) => { | ||
| const fieldDefault = | ||
| this.getFieldInfo(field).instance?.options.defaultValue | ||
| const formDefault = getBy(this.options.defaultValues, field) | ||
| const targetValue = fieldDefault ?? formDefault | ||
|
|
||
| return { | ||
| ...prev, | ||
| fieldMetaBase: { | ||
| ...prev.fieldMetaBase, | ||
| [field]: defaultFieldMeta, | ||
| }, | ||
| values: this.options.defaultValues | ||
| ? setBy(prev.values, field, getBy(this.options.defaultValues, field)) | ||
| : prev.values, | ||
| values: | ||
| targetValue !== undefined | ||
| ? setBy(prev.values, field, targetValue) | ||
| : prev.values, | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| 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: { | ||
|
|
||
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.
Recheck semver level for this behavioral precedence change.
This changes user-visible semantics of
isDefaultValue,reset(), andresetField(); consumers depending on previous dual-default behavior may break. Please confirm whether this should be amajorbump (or feature-flagged), notminor.🤖 Prompt for AI Agents