-
-
Notifications
You must be signed in to change notification settings - Fork 3k
feat: loader.createSchema()
#14759
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
feat: loader.createSchema()
#14759
Conversation
🦋 Changeset detectedLatest commit: c0be1db 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 |
…ub.com/withastro/astro into feat/content-loader-get-schema-context
📝 Changeset Validation Results❌ Changeset validation failed Issues Found:
|
|
@delucis I'd like your review on this when you have time |
Small question purely based on reading the PR description so far. The "3rd party ones that use a schema without a user schema (types will be
|
|
No for static schemas, types will be
|
packages/astro/test/fixtures/content-layer/src/loaders/post-loader.ts
Outdated
Show resolved
Hide resolved
ascorbic
left a comment
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 still not really convinced about the value of having defineContentLoader. These sort of helpers are useful when a user is defining a config object and exporting it directly. However the pattern that we encourage for loader authors is to create a function that returns a loader. This is similar to how Vite provides defineConfig but not definePlugin. I don't really see the value of the helper vs a return type and generic. I'm concerned that the helper just makes it feel like more of a breaking change than it really is.
One day we'll want to make another breaking change to types and we'll run in the same situation as today: it breaks until authors update their types. If we have this type helper, that means we can introduce breaking changes more easily in the future while making it easy for content authors. That's a big advantage IMO. For people looking through this, here are the 2 options basically: // OPTION 1
const schema = z.object({/* ... */})
export function myLoader(options): Loader<typeof schema> {
return {
name: '...',
schema,
async load() {}
}
}
// OPTION 2
export functon myLoader(options) {
return defineContentLoader({
name: '...',
schema: z.object({/* ... */}),
async load() {}
})
} |
|
I think I side with @ascorbic here. If Astro is going to be a TypeScript project then I think we should embrace typescript features like function return types and I'll acknowledge though that some people disagree and like these things and want their code to not have type annotations. Our job as a framework is to resolve these things in some way that maintains consistency, not to always make case-by-case decisions. So there probably needs to be a discussion outside of this PR on when define() wrappers are appropriate and when they are not. I can recall doing so in the past, but maybe its time to revisit and draw more clear lines. Additionally, I think this PR is maybe overloaded. Can we not move |
|
The other option is to tell users to do: export function myLoader(options){
return {
name: '...',
schema: z.object(),
async load() {}
} satisfies Loader
}That fits with what we do for getStaticPaths etc |
|
Alright I'll go with |
packages/astro/test/fixtures/content-layer/src/content.config.ts
Outdated
Show resolved
Hide resolved
loader.createSchema()
packages/astro/test/fixtures/content-layer/src/loaders/post-loader.ts
Outdated
Show resolved
Hide resolved
packages/astro/test/fixtures/content-layer/src/content.config.ts
Outdated
Show resolved
Hide resolved
sarah11918
left a comment
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.
Some suggestions from me for changeset/error message!
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
Changes
zod-to-ts. It requires quite a few changes:createSchema()property instead. It must return a zod schema as well as types (string). That means loaders authors can usezod-to-tsthemselves to keep the previous behavior (migration path)satisfies Loaderso we can infer types properly. It is kind of backward compatible with v5 loaders: it won't break but types will beany. It only affects loaders that specify a static top level schemaany)Testing
Manual + updated
Docs