-
-
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
Conversation
🦋 Changeset detectedLatest commit: 13887e0 The changes in this PR will be included in the next version bump. 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 |
Curious if there's anything to upstream / open source as a general solution once this merges 😁 looking amazing |
@@ -771,7 +771,7 @@ See https://docs.astro.build/en/guides/server-side-rendering/ for more informati | |||
code: 9001, | |||
message: (collection: string, entryId: string, error: ZodError) => { | |||
return [ | |||
`${String(collection)} → ${String(entryId)} frontmatter does not match collection schema.`, | |||
`**${String(collection)} → ${String(entryId)}** frontmatter is invalid.`, |
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 👍
@jasikpark Already want to bring this to our RSS config handler! Also throws a big ole |
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 is CLEAN 👏 quick glance over looks great to me, though I haven't tested or thought through the logic
reads similar to https://github.com/causaly/zod-validation-error - maybe there's interesting prior art there for how to generalize this, tho ig it probably looks exactly the same lol |
@jasikpark I gave that library a try! Didn't like that keys were listed at the end of an error instead of the beginning (i.e. |
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.
Left a comment! I might try to go for more surprise than judgement, but non-blocking!
@bholmesdev yep didn't think it'd be a drop-in at all, just wanted to make you aware / show that there's existing interest in the concept of "give me pretty error messages from a zod validation error" |
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.
That's a great improvement, nice work @bholmesdev! Docs LGTM 🙌
Changes
This fixes a few problems in today's error formatter:
Example: a union between
{ key: z.literal('tutorial') }
and{ key: z.literal('blog') }
will raise a single error whenkey
does not match:Did not match union. > key: Expected `'tutorial' | 'blog'`, received 'foo'
Before / after
Here is a before and after of the error experience when authoring in docs.astro.build. These errors are based on the union schema configured here.
Example invalid frontmatter:
Before
After
Testing
Docs