Skip to content
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] Allow Zod unions, objects, and transforms as schemas #5770

Merged
merged 7 commits into from
Jan 10, 2023

Conversation

bholmesdev
Copy link
Contributor

@bholmesdev bholmesdev commented Jan 5, 2023

Changes

  • Allow Zod objects, unions, discriminated unions, intersections, and transform results as schemas
  • Add helpful warning when z.object(...) wrapper is missing

Why?

By enforcing the z.object(...) wrapper, we open the door for:

  • using a transform to add and modify frontmatter properties
const blog = defineCollection({
  schema: z.object({
    youtube: z.string().url(),
  }).transform(data => ({
    embedUrl: toEmbedUrl(data.youtube),
    thumbnailUrl: toThumbnailUrl(data.youtube),
  })
});
  • using unions and discriminated unions for supporting multiple schemas on the same collection. This is common when splitting multiple options based on a type property:
const posts = defineCollection({
	schema: z.discriminatedUnion('type', [
		z.object({
			type: z.literal('post'),
			title: z.string(),
			description: z.string(),
		}),
		z.object({
			type: z.literal('newsletter'),
			subject: z.string(),
		}),
	]),
});

Testing

Add a test for Zod unions

Docs

withastro/docs#2335

@changeset-bot
Copy link

changeset-bot bot commented Jan 5, 2023

⚠️ No Changeset found

Latest commit: 8c4ef10

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pkg: example Related to an example package (scope) labels Jan 5, 2023
@matthewp
Copy link
Contributor

matthewp commented Jan 5, 2023

Why do we have to require the use of z.object() to get those things?

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely in favor of supporting this but I'm not convinced we need to deprecate the current raw object support?

Comment on lines +72 to +75
if (
typeof collectionConfig.schema === 'object' &&
!('safeParseAsync' in collectionConfig.schema)
) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we could allow POJOs (plain ole' JS objects) here and automatically wrap them with z.object if necessary? Small convenience if you don't need any of the advanced z.object features.

@bholmesdev
Copy link
Contributor Author

@matthewp @natemoo-re Alright, reasons I'm in favor of deprecating the raw object:

Requiring z.object makes makes shared schemas easier to document. Allowing POJOs today, we encourage this pattern:

const schema1 = {...}
const schema2 = {...}

const blog = defineCollection({
  schema: { ...schema1, ...schema2, moreProperties: true },
});

Now, say schema1 needs a transform. schema1 would need to change from a POJO to an object:

const schema1 = z.object({ ... }).transform(...)
const schema2 = {...}

const blog = defineCollection({
  schema: ???,
});

...and now schema1 and schema2 are difficult to combine. This problem is more pronounced when schemas come from third party libraries.

Requiring schemas to be Zod types, we can always document and encourage a union like so:

const schema1 = z.object({ ... }).transform(...)
const schema2 = z.object({ ... })

const blog = defineCollection({
  schema: z.union([schema1, schema2, z.object({ moreProperties: true })]),
});

I'm also thinking about how much we'd explain if we document transforms or unions. Allowing POJOs, we'd need an "if you're using an object, change to a Zod object first" warning that'd be nice to avoid.

These are fairly minor complaints I know! I just prefer a single standard for doing things to make features discoverable and easy-to-document.

@delucis
Copy link
Member

delucis commented Jan 6, 2023

+1 to a single consistent API here. Omitting the z.object() wrapper is a fairly small aesthetic benefit but sounds like it would increase documentation complexity significantly.

@bholmesdev bholmesdev changed the title [Content Collections] Require z.object(...) wrapper for schemas [Content Collections] Allow Zod unions, objects, and transforms as schemas Jan 6, 2023
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been convinced! It's a bit boilerplate-y for the simple use case, but I agree that increasing the documentation complexity isn't worth it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pkg: example Related to an example package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants