fix(form-core): establish Field-over-Form prioritization for isDefaultValue and resets#2006
Conversation
🦋 Changeset detectedLatest commit: 699134c The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
View your CI Pipeline Execution ↗ for commit 51d5019
☁️ Nx Cloud last updated this comment at |
Kyujenius
force-pushed
the
fix/isDefaultValue-logic
branch
from
3253468 to
9dead10
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2006 +/- ##
==========================================
+ Coverage 90.35% 90.76% +0.41%
==========================================
Files 38 59 +21
Lines 1752 2209 +457
Branches 444 555 +111
==========================================
+ Hits 1583 2005 +422
- Misses 149 183 +34
- Partials 20 21 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
LeCarbonator
left a comment
LeCarbonator
left a comment
There was a problem hiding this comment.
Looks clean! I'll need to do some additional checks though, since this is potentially a breaking change instead of a fix.
It may take a few days. Thanks for tackling the issue!
|
|
||
| field.setValue('test') | ||
| expect(field.getMeta().isDefaultValue).toBe(true) | ||
| expect(field.getMeta().isDefaultValue).toBe(false) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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 Interpretations
Original (OR logic): "Both are defaults"
isDefaultValue = matches(formDefault) || matches(fieldDefault)
Proposed (?? logic): "The one reset() uses is THE default"
isDefaultValue = matches(fieldDefault ?? formDefault)
Why I Lean Toward the Proposed Logic
In #1081, explained the purpose of isDefaultValue:
"if you do something like
!meta.isDefaultValuewill give you the react ecosystem's isDirty"
RHF's isDirty is true when currentValue !== defaultValue. A form has exactly one clean state.
With the original OR logic:
isDefaultValueistruefor both'test'and'another-test'- So
!isDefaultValueisfalsefor both values - This means the form has two clean states
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 'test' (form-level default):
isDefaultValue→true→ user skips reset- But
reset()would actually change the value to'another-test'
The user gets misleading information.
I Could Be Wrong
I 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?
📝 WalkthroughWalkthroughThis PR implements a prioritized default-value system for TanStack Form, enforcing field-level defaults over form-level defaults across ChangesPrioritized Default System
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 2
🤖 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 @.changeset/gentle-jars-share.md:
- Around line 2-5: The changeset claims a minor bump but introduces behavioral
precedence altering user-visible semantics (Prioritized Default System)
affecting isDefaultValue, form.reset(), and form.resetField(); re-evaluate and
change the changeset to a major version bump or gate the behavior behind a
feature flag/opt-in, update the .changeset text to reflect the decision, and
ensure release notes explicitly document the behavioral change and migration
guidance for consumers relying on previous dual-default behavior.
In `@packages/form-core/src/FormApi.ts`:
- Around line 1135-1140: The code uses nullish coalescing (??) so an explicit
field default of null is treated as missing and falls back to form defaults;
update the logic in the evaluate call to treat only undefined as "missing"
(preserve null/false/0 as valid defaults). Concretely, retrieve the
field-specific default via
this.getFieldInfo(fieldName)?.instance?.options.defaultValue into a local (e.g.,
fieldDefault) and use fieldDefault !== undefined ? fieldDefault :
getBy(this.options.defaultValues, fieldName) when calling evaluate (change the
occurrence inside evaluate and the similar occurrences around the 2601-2605
area) so explicit null values retain precedence.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b1d96c6d-8ef2-4650-b68b-d363a041a5ea
📒 Files selected for processing (4)
.changeset/gentle-jars-share.mdpackages/form-core/src/FormApi.tspackages/form-core/tests/FieldApi.spec.tspackages/form-core/tests/FormApi.spec.ts
| '@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. |
There was a problem hiding this comment.
Recheck semver level for this behavioral precedence change.
This changes user-visible semantics of isDefaultValue, reset(), and resetField(); consumers depending on previous dual-default behavior may break. Please confirm whether this should be a major bump (or feature-flagged), not minor.
🤖 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 @.changeset/gentle-jars-share.md around lines 2 - 5, The changeset claims a
minor bump but introduces behavioral precedence altering user-visible semantics
(Prioritized Default System) affecting isDefaultValue, form.reset(), and
form.resetField(); re-evaluate and change the changeset to a major version bump
or gate the behavior behind a feature flag/opt-in, update the .changeset text to
reflect the decision, and ensure release notes explicitly document the
behavioral change and migration guidance for consumers relying on previous
dual-default behavior.
| 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, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
null field defaults lose precedence due to ??.
Using nullish coalescing here makes explicit defaultValue: null fall back to form defaults, which breaks Field-over-Form priority and can desynchronize reset/default metadata behavior.
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
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/form-core/src/FormApi.ts` around lines 1135 - 1140, The code uses
nullish coalescing (??) so an explicit field default of null is treated as
missing and falls back to form defaults; update the logic in the evaluate call
to treat only undefined as "missing" (preserve null/false/0 as valid defaults).
Concretely, retrieve the field-specific default via
this.getFieldInfo(fieldName)?.instance?.options.defaultValue into a local (e.g.,
fieldDefault) and use fieldDefault !== undefined ? fieldDefault :
getBy(this.options.defaultValues, fieldName) when calling evaluate (change the
occurrence inside evaluate and the similar occurrences around the 2601-2605
area) so explicit null values retain precedence.
crutchcorn
left a comment
crutchcorn
left a comment
There was a problem hiding this comment.
This looks really good. Thank you for the thoughtful PR and description; sorry that it took me so long to review + merge.
Just as a heads up, @LeCarbonator is considering some changes to defaultValue in a field for v2. Namely removing it outright. Would love to hear your feedback on that idea as someone who's clearly thought about field-first default values.
|
@crutchcorn Thanks so much, and no worries on the timing! I’ve picked up on bits of the related conversations around field-level vs form-level defaults, so dropping field-level defaultValue in v2 sounds like a reasonable direction from where I’m sitting. Would love to contribute to that effort in whatever capacity is useful — just let me know where the discussion lives and I’ll join in. |
3 participants
🎯 Changes
Fixes : #1973
This PR is built on the fundamental premise that "The most specific scope (Field) must always take precedence over the generic scope (Form)." Similar to variable shadowing in programming or specificity in CSS, an explicit declaration at the field level represents a stronger developer intent than a global default.
Currently, the lack of a clear hierarchy leads to architectural inconsistencies:
isDefaultValuemight claim a value is "default," yetreset()restores a completely different value from the form-level config.undefinedincorrectly triggersisDefaultValue: trueeven when explicit defaults are defined at the form level.By unifying
isDefaultValue,form.reset(), andform.resetField()under a single Prioritization Strategy (Field over Form), this PR ensures that TanStack Form behaves as a predictable, high-integrity state machine.Introduction of Prioritized Default System
This PR introduces a unified prioritization strategy where Field-level defaults always override Form-level defaults.
Key Changes:
isDefaultValueDetermination: Now uses a single prioritized default value (Field ?? Form) for comparison instead of a logical OR.form.reset()Consistency: Now merges form-level defaults with currently mounted field-level defaults, preventing the "reset betrayal" where values would unexpectedly change to form-defaults.form.resetField()Consistency: Updated to respect the same priority when resetting individual fields.Fixed Scenarios:
undefinednow correctly setsisDefaultValue: false.isDefaultValueandreset()now consistently follow the field-level default.I’d like to verify if this matches what the maintainers had in mind.
Modified current test cases in
FieldApi.spec.tsthat were previously based on the incorrect logical OR assumption. These tests were updated to reflect the new prioritized logic, ensuring thatisDefaultValueonly returnstruefor the actual value thatform.reset()would restore.isDefaultValue Check Logic AS-IS / TO-BE
resset() Logic AS-IS / TO-BE
IMO
I realize this shifts the precedence closer to a 'Variable Shadowing' model. If this is too big of a breaking change, I'm happy to discuss putting this behind a flag or finding a middle ground. My main goal is to ensure reset() and isDefaultValue rely on the same logic. I believe this prioritization makes the library significantly more predictable. By aligning the internal logic with how developers naturally think about scope and specificity,
✅ Checklist
pnpm test:pr. (Verifiedform-coretests and custom regression tests)🚀 Release Impact
Summary by CodeRabbit
New Features
Bug Fixes