-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[Content collections] Better error formatting #6508
Merged
Merged
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
a1e1bc8
feat: new error map with unions, literal, and fbs
bholmesdev 0c4fcd7
fix: bad type casting on type error
bholmesdev 25e9b53
refactor: add comments, clean up edge cases
bholmesdev 4df3431
refactor: JSON.stringify, add colon for follow ups
bholmesdev ed0a7f3
refactor: bold file name in msg
bholmesdev cd7a33d
fix: prefix union errors
bholmesdev c5ab794
test: new error mapping
bholmesdev 40cb080
refactor: clean up Required handling
bholmesdev 732cb31
test: required, fallthrough
bholmesdev 0e92769
chore: changeset
bholmesdev 3b35ad3
fix: out-of-date error msg
bholmesdev 72b4a20
chore: stray console log
bholmesdev 13887e0
docs: revert to old error msg
bholmesdev File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
--- | ||
'astro': patch | ||
--- | ||
|
||
Improve content collection error formatting: | ||
- Bold the collection and entry that failed | ||
- Consistently list the frontmatter key at the start of every error | ||
- Rich errors for union types |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
import type { ZodErrorMap } from 'zod'; | ||
|
||
type TypeOrLiteralErrByPathEntry = { | ||
code: 'invalid_type' | 'invalid_literal'; | ||
received: unknown; | ||
expected: unknown[]; | ||
}; | ||
|
||
export const errorMap: ZodErrorMap = (baseError, ctx) => { | ||
const baseErrorPath = flattenErrorPath(baseError.path); | ||
if (baseError.code === 'invalid_union') { | ||
// Optimization: Combine type and literal errors for keys that are common across ALL union types | ||
// Ex. a union between `{ key: z.literal('tutorial') }` and `{ key: z.literal('blog') }` will | ||
// raise a single error when `key` does not match: | ||
// > Did not match union. | ||
// > key: Expected `'tutorial' | 'blog'`, received 'foo' | ||
let typeOrLiteralErrByPath: Map<string, TypeOrLiteralErrByPathEntry> = new Map(); | ||
for (const unionError of baseError.unionErrors.map((e) => e.errors).flat()) { | ||
if (unionError.code === 'invalid_type' || unionError.code === 'invalid_literal') { | ||
const flattenedErrorPath = flattenErrorPath(unionError.path); | ||
if (typeOrLiteralErrByPath.has(flattenedErrorPath)) { | ||
typeOrLiteralErrByPath.get(flattenedErrorPath)!.expected.push(unionError.expected); | ||
} else { | ||
typeOrLiteralErrByPath.set(flattenedErrorPath, { | ||
code: unionError.code, | ||
received: unionError.received, | ||
expected: [unionError.expected], | ||
}); | ||
} | ||
} | ||
} | ||
let messages: string[] = [ | ||
prefix( | ||
baseErrorPath, | ||
typeOrLiteralErrByPath.size ? 'Did not match union:' : 'Did not match union.' | ||
), | ||
]; | ||
return { | ||
message: messages | ||
.concat( | ||
[...typeOrLiteralErrByPath.entries()] | ||
// If type or literal error isn't common to ALL union types, | ||
// filter it out. Can lead to confusing noise. | ||
.filter(([, error]) => error.expected.length === baseError.unionErrors.length) | ||
.map(([key, error]) => | ||
key === baseErrorPath | ||
? // Avoid printing the key again if it's a base error | ||
`> ${getTypeOrLiteralMsg(error)}` | ||
: `> ${prefix(key, getTypeOrLiteralMsg(error))}` | ||
) | ||
) | ||
.join('\n'), | ||
}; | ||
} | ||
if (baseError.code === 'invalid_literal' || baseError.code === 'invalid_type') { | ||
return { | ||
message: prefix( | ||
baseErrorPath, | ||
getTypeOrLiteralMsg({ | ||
code: baseError.code, | ||
received: baseError.received, | ||
expected: [baseError.expected], | ||
}) | ||
), | ||
}; | ||
} else if (baseError.message) { | ||
return { message: prefix(baseErrorPath, baseError.message) }; | ||
} else { | ||
return { message: prefix(baseErrorPath, ctx.defaultError) }; | ||
} | ||
}; | ||
|
||
const getTypeOrLiteralMsg = (error: TypeOrLiteralErrByPathEntry): string => { | ||
if (error.received === 'undefined') return 'Required'; | ||
const expectedDeduped = new Set(error.expected); | ||
switch (error.code) { | ||
case 'invalid_type': | ||
return `Expected type \`${unionExpectedVals(expectedDeduped)}\`, received ${JSON.stringify( | ||
error.received | ||
)}`; | ||
case 'invalid_literal': | ||
return `Expected \`${unionExpectedVals(expectedDeduped)}\`, received ${JSON.stringify( | ||
error.received | ||
)}`; | ||
} | ||
}; | ||
|
||
const prefix = (key: string, msg: string) => (key.length ? `**${key}**: ${msg}` : msg); | ||
|
||
const unionExpectedVals = (expectedVals: Set<unknown>) => | ||
[...expectedVals] | ||
.map((expectedVal, idx) => { | ||
if (idx === 0) return JSON.stringify(expectedVal); | ||
const sep = ' | '; | ||
return `${sep}${JSON.stringify(expectedVal)}`; | ||
}) | ||
.join(''); | ||
|
||
const flattenErrorPath = (errorPath: (string | number)[]) => errorPath.join('.'); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
107 changes: 107 additions & 0 deletions
107
packages/astro/test/units/content-collections/error-map.test.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,107 @@ | ||
import { z } from '../../../zod.mjs'; | ||
import { errorMap } from '../../../dist/content/index.js'; | ||
import { fixLineEndings } from '../../test-utils.js'; | ||
import { expect } from 'chai'; | ||
|
||
describe('Content Collections - error map', () => { | ||
it('Prefixes messages with object key', () => { | ||
const error = getParseError( | ||
z.object({ | ||
base: z.string(), | ||
nested: z.object({ | ||
key: z.string(), | ||
}), | ||
union: z.union([z.string(), z.number()]), | ||
}), | ||
{ base: 1, nested: { key: 2 }, union: true } | ||
); | ||
const msgs = messages(error).sort(); | ||
expect(msgs).to.have.length(3); | ||
// expect "**" for bolding | ||
expect(msgs[0].startsWith('**base**')).to.equal(true); | ||
expect(msgs[1].startsWith('**nested.key**')).to.equal(true); | ||
expect(msgs[2].startsWith('**union**')).to.equal(true); | ||
}); | ||
it('Returns formatted error for type mismatch', () => { | ||
const error = getParseError( | ||
z.object({ | ||
foo: z.string(), | ||
}), | ||
{ foo: 1 } | ||
); | ||
expect(messages(error)).to.deep.equal(['**foo**: Expected type `"string"`, received "number"']); | ||
}); | ||
it('Returns formatted error for literal mismatch', () => { | ||
const error = getParseError( | ||
z.object({ | ||
lang: z.literal('en'), | ||
}), | ||
{ lang: 'es' } | ||
); | ||
expect(messages(error)).to.deep.equal(['**lang**: Expected `"en"`, received "es"']); | ||
}); | ||
it('Replaces undefined errors with "Required"', () => { | ||
const error = getParseError( | ||
z.object({ | ||
foo: z.string(), | ||
bar: z.string(), | ||
}), | ||
{ foo: 'foo' } | ||
); | ||
expect(messages(error)).to.deep.equal(['**bar**: Required']); | ||
}); | ||
it('Returns formatted error for basic union mismatch', () => { | ||
const error = getParseError( | ||
z.union([z.boolean(), z.number()]), | ||
'not a boolean or a number, oops!' | ||
); | ||
expect(messages(error)).to.deep.equal([ | ||
fixLineEndings( | ||
'Did not match union:\n> Expected type `"boolean" | "number"`, received "string"' | ||
), | ||
]); | ||
}); | ||
it('Returns formatted error for union mismatch on nested object properties', () => { | ||
const error = getParseError( | ||
z.union([ | ||
z.object({ | ||
type: z.literal('tutorial'), | ||
}), | ||
z.object({ | ||
type: z.literal('article'), | ||
}), | ||
]), | ||
{ type: 'integration-guide' } | ||
); | ||
expect(messages(error)).to.deep.equal([ | ||
fixLineEndings( | ||
'Did not match union:\n> **type**: Expected `"tutorial" | "article"`, received "integration-guide"' | ||
), | ||
]); | ||
}); | ||
it('Lets unhandled errors fall through', () => { | ||
const error = getParseError( | ||
z.object({ | ||
lang: z.enum(['en', 'fr']), | ||
}), | ||
{ lang: 'jp' } | ||
); | ||
expect(messages(error)).to.deep.equal([ | ||
"**lang**: Invalid enum value. Expected 'en' | 'fr', received 'jp'", | ||
]); | ||
}); | ||
}); | ||
|
||
/** | ||
* @param {z.ZodError} error | ||
* @returns string[] | ||
*/ | ||
function messages(error) { | ||
return error.errors.map((e) => e.message); | ||
} | ||
|
||
function getParseError(schema, entry, parseOpts = { errorMap }) { | ||
const res = schema.safeParse(entry, parseOpts); | ||
expect(res.success).to.equal(false, 'Schema should raise error'); | ||
return res.error; | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Flagging for docs review @sarah11918 👀
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.
@bholmesdev I love the change overall! And, I know you wanted something shorter here, but I don't love "invalid" vs "does not match what you said you wanted here." (which is why it's invalid)
If you feel the need to shorten, something like ...
"unexpected frontmatter" feels a little more user friendly?
Your call! Non-blocking!
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.
Understood! Didn't feel too strongly about this change. I think I'll revert to the old wording, with the bolding added 👍