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

Add note about ids having to be strings to content collection docs #10612

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

louisescher
Copy link
Contributor

Description (required)

This PR adds a sentence to clarify that the IDs of content collections always have to be strings. This is undocumented behavior and lead to confusion, see Custom content loader: entry missing id when using an id from the source data on the Astro Discord server.

Related issues & labels (optional)

Copy link

netlify bot commented Jan 5, 2025

Deploy Preview for astro-docs-2 ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 016fd4b
🔍 Latest deploy log https://app.netlify.com/sites/astro-docs-2/deploys/6779dd8102a52000089431a3
😎 Deploy Preview https://deploy-preview-10612--astro-docs-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@astrobot-houston
Copy link
Contributor

Lunaria Status Overview

🌕 This pull request will trigger status changes.

Learn more

By default, every PR changing files present in the Lunaria configuration's files property will be considered and trigger status changes accordingly.

You can change this by adding one of the keywords present in the ignoreKeywords property in your Lunaria configuration file in the PR's title (ignoring all files) or by including a tracker directive in the merged commit's description.

Tracked Files

File Note
en/guides/content-collections.mdx Source changed, localizations will be marked as outdated.
Warnings reference
Icon Description
🔄️ The source for this localization has been updated since the creation of this pull request, make sure all changes in the source have been applied.

@@ -223,7 +223,7 @@ Schemas enforce consistent frontmatter or entry data within a collection through

Schemas also power Astro's automatic TypeScript typings for your content. When you define a schema for your collection, Astro will automatically generate and apply a TypeScript interface to it. The result is full TypeScript support when you query your collection, including property autocompletion and type-checking.

Every frontmatter or data property of your collection entries must be defined using a Zod data type:
Every frontmatter or data property of your collection entries must be defined using a Zod data type. When defining your schema, make sure that if you provide an `id` property, it is a string:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should say that an id field is required, so if you provide a schema it must include the id field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ascorbic Would

When defining your schema, make sure that you provide an `id` property and that it is a string:

be fine?

@@ -223,7 +223,7 @@ Schemas enforce consistent frontmatter or entry data within a collection through

Schemas also power Astro's automatic TypeScript typings for your content. When you define a schema for your collection, Astro will automatically generate and apply a TypeScript interface to it. The result is full TypeScript support when you query your collection, including property autocompletion and type-checking.

Every frontmatter or data property of your collection entries must be defined using a Zod data type:
Every frontmatter or data property of your collection entries must be defined using a Zod data type. When defining your schema, make sure that if you provide an `id` property, it is a string:
Copy link
Member

@sarah11918 sarah11918 Jan 6, 2025

Choose a reason for hiding this comment

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

This feels a little off here. This is currently talking about the schema including "Every frontmatter or data property" (ie properties of the data itself) and the example below does not show defining id. In fact, when we describe the built in glob loader we say id is automatically generated, but we say nothing about needing to define it in a schema. So, it's confusing (even to me!) -- obviously you don't always put the id in the schema (we didn't for glob(). So, when do you need to put it in the schema?)

In any case, I think we could intro this a bit better with something like:

Suggested change
Every frontmatter or data property of your collection entries must be defined using a Zod data type. When defining your schema, make sure that if you provide an `id` property, it is a string:
Every frontmatter or data property of your collection entries must be defined using a Zod data type. Your content loader may provide this schema for you based on the data you are importing. If not, you must define your own schema as a property of your collection.
The example below shows defining a schema based on frontmatter properties in local Markdown files that are loaded by Astro's built-in `glob()` loader:

This adds the idea that some loaders define the schema for you. But, even in the example below, we're defining a schema but not id.

So it feels weird to describe defining id when that's not a part of the basic intro to what schemas are. We need some kind of linkage, something like (this is not correct because I don't know when this is necessary):

In addition to defining properties of your data entries in your schema, you can type the entry id itself as z.string(). This may be useful/necessary when the entry is not automatically generated for you by your loader...."

And, maybe this even comes AFTER the code example if it's kind of an edge case?

Again, my issue is that it's confusing to talk about typing id when our example does not show doing that, so this can't just be one sentence tacked on but needs some smoothing to make it make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sarah11918 I do agree, it feels off to put this here. However, at no point in the docs is it mentioned that the ID must be a string. That's really the intent behind this change, but I had no better spot to put this. Maybe this is just a case of saying nothing would be better since the PR to Astro improves the error?

Copy link
Member

@sarah11918 sarah11918 Jan 6, 2025

Choose a reason for hiding this comment

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

@louisescher I agree, and looking at the error message, it's less about defining your id as a string in the schema and more about making sure IDs are strings, I think?

I think the error message is sufficient and would suggest closing this PR, I think? How about you?

EDIT:

Should we maybe be specifying something here instead? I noticed we show examples in this section, but don't explicity state the type of string, for example: https://docs.astro.build/en/reference/modules/astro-content/#id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with the location you proposed in your edit, that seems like a nice spot to put it. I'll revert the original change and add a sentence to that part, as well as changing the "Example Type: ..." to "Type: string". Would that be OK?

Copy link
Member

@sarah11918 sarah11918 Jan 6, 2025

Choose a reason for hiding this comment

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

If you're going to document a type, then you need @ArmandPhilippot 's permission, not mine! 😅

Here's the official pattern we use: https://contribute.docs.astro.build/reference/api-references/

(But yes, I would say revert what you've got on the Content Collections guide page itself!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants