-
-
Notifications
You must be signed in to change notification settings - Fork 527
fix(form-core): make fieldMeta values optional to reflect runtime behaviour. #1787
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?
Conversation
…avior fieldMeta is typed as Record<DeepKeys<TData>, AnyFieldMeta> but is initialized as an empty object. This causes TypeScript to incorrectly assume that accessing any valid field key will return a defined AnyFieldMeta object, leading to runtime crashes when accessing field metadata during the first render. Updated the fieldMeta type to include undefined to accurately reflect that field metadata is only available after a field has been mounted. BREAKING CHANGE: fieldMeta values are now typed as potentially undefined. Code that accesses fieldMeta without null checks will now show TypeScript errors. Use optional chaining or explicit undefined checks. Fixes TanStack#1774
🦋 Changeset detectedLatest commit: 39f1bf1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 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 39f1bf1
☁️ Nx Cloud last updated this comment at |
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.
Looks good! I like the unit tests to prevent regression. There's a few things to clean up, but as far as the type changes go for FormApi, it looks fine by me!
.changeset/bumpy-boats-roll.md
Outdated
@@ -0,0 +1,27 @@ | |||
--- | |||
'@tanstack/form-core': major |
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.
We don't consider type changes a breaking change, especially not ones like this that try to prevent existing runtime errors from being undetected.
See https://tanstack.com/form/latest/docs/typescript for more info.
'@tanstack/form-core': major | |
'@tanstack/form-core': patch |
Make fieldMeta values optional to reflect runtime behavior and prevent crashes | ||
|
||
BREAKING CHANGE: `fieldMeta` values are now typed as `Record<DeepKeys<TData>, AnyFieldMeta | undefined>` instead of `Record<DeepKeys<TData>, AnyFieldMeta>`. This accurately reflects that field metadata is only available after a field has been mounted. | ||
|
||
**Why:** Previously, TypeScript allowed unchecked access to `fieldMeta` properties, leading to runtime crashes when accessing metadata of unmounted fields during the first render. | ||
|
||
**What changed:** The type now includes `undefined` in the union, forcing developers to handle the case where a field hasn't been mounted yet. | ||
|
||
**How to migrate:** | ||
|
||
```typescript | ||
// Before (crashes at runtime) | ||
const isValid = form.state.fieldMeta.name.isValid | ||
|
||
// After - use optional chaining | ||
const isValid = form.state.fieldMeta.name?.isValid | ||
|
||
// Or explicit undefined check | ||
const fieldMeta = form.state.fieldMeta.name | ||
if (fieldMeta) { | ||
const isValid = fieldMeta.isValid | ||
} |
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.
I'm not sure how GH actions will handle descriptions like this. I assume it'll be okay? Either way, as mentioned above, types aren't considered breaking changes.
it('should not crash when accessing properties on undefined fieldMeta with optional chaining', () => { | ||
const form = new FormApi({ | ||
defaultValues: { | ||
name: '', | ||
email: '', | ||
}, | ||
}) | ||
|
||
expect(() => { | ||
const isValid = form.state.fieldMeta.name?.isValid | ||
const isTouched = form.state.fieldMeta.name?.isTouched | ||
const errors = form.state.fieldMeta.name?.errors | ||
return { isValid, isTouched, errors } | ||
}).not.toThrow() | ||
|
||
expect(form.state.fieldMeta.name?.isValid).toBeUndefined() | ||
expect(form.state.fieldMeta.name?.isTouched).toBeUndefined() | ||
}) |
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.
This unit test is unrelated to this library. Optional chaining is already part of JavaScript spec. Please remove this redundant test.
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.
*.spec.ts
files imply they're runtime tests, while type tests go into *.test-d.ts
.
- Please rename the file to
fieldMeta.spec.ts
- Change the
describe
string since the file tests for meta accessing and not type safety.
it('should allow conditional access patterns', () => { | ||
const form = new FormApi({ | ||
defaultValues: { | ||
name: '', | ||
}, | ||
}) | ||
|
||
const isValid1 = form.state.fieldMeta.name?.isValid ?? true | ||
expect(isValid1).toBe(true) | ||
|
||
const fieldMeta = form.state.fieldMeta.name | ||
let isValid2 = true | ||
if (fieldMeta) { | ||
isValid2 = fieldMeta.isValid | ||
} | ||
expect(isValid2).toBe(true) | ||
|
||
const errors = form.state.fieldMeta.name?.errors ?? [] | ||
expect(errors).toEqual([]) | ||
}) |
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.
This unit test is unrelated to the library. Nullish coalescing is already part of JavaScript spec. Please remove it.
it('should handle Object.values on fieldMeta safely', () => { | ||
const form = new FormApi({ | ||
defaultValues: { | ||
field1: '', | ||
field2: '', | ||
field3: '', | ||
}, | ||
}) | ||
|
||
const field1 = new FieldApi({ | ||
form, | ||
name: 'field1', | ||
}) | ||
|
||
field1.mount() | ||
|
||
const fieldMetaValues = Object.values(form.state.fieldMeta).filter(Boolean) | ||
expect(fieldMetaValues.length).toBeGreaterThan(0) | ||
expect(fieldMetaValues.every((meta) => meta !== undefined)).toBe(true) | ||
}) | ||
|
||
it('should type-check correctly with TypeScript', () => { | ||
const form = new FormApi({ | ||
defaultValues: { | ||
name: '', | ||
}, | ||
}) | ||
|
||
const meta = form.state.fieldMeta.name | ||
|
||
if (meta) { | ||
const isValid: boolean = meta.isValid | ||
const errors: unknown[] = meta.errors | ||
expect(isValid).toBeDefined() | ||
expect(errors).toBeDefined() | ||
} | ||
|
||
const isValid = form.state.fieldMeta.name?.isValid | ||
const errors = form.state.fieldMeta.name?.errors | ||
|
||
expect(isValid === undefined || typeof isValid === 'boolean').toBe(true) | ||
expect(errors === undefined || Array.isArray(errors)).toBe(true) | ||
}) |
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.
These two tests are already covered by ones above, so they're not really needed.
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.
Thanks for the feedback! I've made the changes. ❤️
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1787 +/- ##
==========================================
+ Coverage 90.35% 90.44% +0.09%
==========================================
Files 38 38
Lines 1752 1790 +38
Branches 444 453 +9
==========================================
+ Hits 1583 1619 +36
- Misses 149 151 +2
Partials 20 20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Fixes #1774
This PR addresses a type safety issue where
fieldMeta
is typed asRecord<DeepKeys<TData>, AnyFieldMeta>
but is initialized as an empty object, causing runtime crashes when accessing field metadata during the first render.Changes
FormState
interface to includeundefined
in fieldMeta typeType of Change
Breaking Changes
This is a breaking change for TypeScript users. Code that previously accessed fieldMeta without null checks will now show type errors, preventing runtime crashes.
Migration Example: