fix(form-core): handle string array indices in prefixSchemaToErrors #1689
Conversation
- Fix array path handling for Standard Schema validators - Support both numeric (Zod) and string (Yup) array indices Fixes TanStack#1683
|
View your CI Pipeline Execution ↗ for commit b3f6cd1
☁️ Nx Cloud last updated this comment at |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1689 +/- ##
=======================================
Coverage ? 90.58%
=======================================
Files ? 37
Lines ? 1689
Branches ? 422
=======================================
Hits ? 1530
Misses ? 142
Partials ? 17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| const isArrayIndex = | ||
| typeof normalizedSegment === 'number' || | ||
| (typeof normalizedSegment === 'string' && | ||
| /^\d+$/.test(normalizedSegment)) | ||
|
|
||
| return isArrayIndex ? `[${normalizedSegment}]` : normalizedSegment | ||
| }) |
There was a problem hiding this comment.
This breaks for numeric properties (example: foo.01234.bar). Ideally, we also need unit tests to ensure they don't break with this implementation.
However, the standard schema validator has access to the form value it validated, so we can use it as reference to see if the access is an array or not.
Something like this:
function prefixSchemaToErrors(
issues: readonly StandardSchemaV1Issue[],
formValue: unknown,
) {
const schema = new Map<string, StandardSchemaV1Issue[]>()
for (const issue of issues) {
const issuePath = issue.path ?? []
let currentFormValue = formValue
let path = ''
for (let i = 0; i < issuePath.length; i++) {
const pathSegment = issuePath[i]!
const segment =
typeof pathSegment === 'object' ? pathSegment.key : pathSegment
// Standard Schema doesn't specify if paths should use numbers or stringified numbers for array access.
// However, if we follow the path it provides and encounter an array, then we can assume it's intended for array access.
const segmentAsNumber = Number(segment)
if (Array.isArray(currentFormValue) && !isNaN(segmentAsNumber)) {
path += `[${segmentAsNumber}]`
} else {
path += (i > 0 ? '.' : '') + String(segment)
}
if (
typeof currentFormValue === 'object' &&
currentFormValue !== null
) {
currentFormValue = currentFormValue[segment as never]
} else {
currentFormValue = undefined
}
}
schema.set(path, (schema.get(path) ?? []).concat(issue))
}
return Object.fromEntries(schema)
}| ]) | ||
| }) | ||
|
|
||
| it('should handle string array indices correctly (Yup compatibility)', async () => { |
There was a problem hiding this comment.
Nitpick, but this is less about Yup being special, and more about standard schema not specifying what a path should contain. It could be any library implementing standard schema.
| const mockYupLikeValidator = { | ||
| '~standard': { | ||
| version: 1 as const, | ||
| vendor: 'mock-yup', | ||
| validate: (value: unknown) => { | ||
| const typedValue = value as { people?: { name?: string }[] } | ||
| if (!typedValue.people?.[0]?.name) { | ||
| return { | ||
| issues: [ | ||
| { | ||
| message: 'Name is required', | ||
| path: ['people', '0', 'name'], // String index like Yup | ||
| }, | ||
| ], | ||
| } | ||
| } | ||
| return { value: typedValue } | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
There's no need to mock a validator.
Zod allows you to set a custom path, allowing us to imitate string paths.
z.object({
people: z.array(z.object({
name: z.string()
}))
}).superRefine((_, ctx) => {
ctx.addIssue({
code: 'custom',
message: 'Name is required',
path: [ 'people', '0', 'name' ],
})
})| }, | ||
| ]) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Failing unit test for the numeric property comment above:
it('should allow numeric object properties for standard schema issue paths', () => {
const form = new FormApi({
defaultValues: {
foo: {
0: { bar: '' },
},
},
validators: {
onChange: z.object({
foo: z.object({
0: z.object({ bar: z.string().email('Must be an email') }),
}),
}),
},
})
form.mount()
const field = new FieldApi({
form,
name: 'foo.0.bar',
})
field.mount()
field.setValue('test')
expect(form.state.errors).toMatchObject([
{ 'foo.0.bar': [{ message: 'Must be an email' }] },
])
expect(field.state.meta.errors).toMatchObject([
{ message: 'Must be an email' },
])
})…validator by inspecting form values
…/form into feat/standard-schema
|
Thank you for the excellent feedback! Your suggestions were spot-on and made the implementation much more robust. I really appreciate you pointing out the numeric object property issue and providing the better approach using actual form values as reference. The I've committed all the changes incorporating your feedback
Please review when you have a chance. Thanks for the great mentorship! |
|
Appreciate the work here, but something made me wonder... in the interest of openness, has this PR been created by AI by any chance? |
|
I wrote the bug fix logic myself. However, I did refer to some reference materials while searching for an efficient way to implement the code. Could this be a problem? |
Description
This PR fixes a bug in the
prefixSchemaToErrorsfunction withinstandardSchemaValidator.tswhere array indices passed as strings (like '0', '1') were not being properly converted to bracket notation, causing validation errors for array fields to be incorrectly formatted.Problem
typeof normalizedSegment === 'number', missing string-based indicespeople.0.nameinstead of the correctpeople[0].nameSolution
/^\d+$/to identify string representations of numbersRelated Issue
Fixes #1683
Type of Change
Testing
Test Results
Code Changes
Before
After
Impact
Checklist