-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[v6] Update content loader reference #12810
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
[v6] Update content loader reference #12810
Conversation
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
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.
Thanks Armand! I had one note so far around the loader object, and wonder whether @HiDeoo has comments for the updated examples in lines 300-600ish
HiDeoo
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.
Left a few comments.
I'm personally not bother at all with the generic types position. If the type of an object accept multiple generic types, I definitely want to know about them immediately, even before knowing what the properties of the object are considering such types could impact the properties.
Co-authored-by: HiDeoo <494699+HiDeoo@users.noreply.github.com>
Co-authored-by: Sarah Rainsberger <5098874+sarah11918@users.noreply.github.com>
Co-authored-by: HiDeoo <494699+HiDeoo@users.noreply.github.com>
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.
Just tiny nits/fixes below, but holy cow is this good!! I get excited just reading it. 😆
(Approving, but leaving the feedback to address as you see fit and then merge when you're happy!)
| }); | ||
| ``` | ||
|
|
||
| ## `astro` types |
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 new section is so good! And so exciting! I am so happy about this!
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.
Glad you like it! It bothered me a bit because this is not astro:content, but yeah I think this makes sense. And, this is better than leaving things undocumented!
We might need similar sections in other modules where we don't show everything importable from astro but only the things relevant to the current API! I guess that leaves me with something to do later. 😆
Co-authored-by: Sarah Rainsberger <5098874+sarah11918@users.noreply.github.com>
|
I pushed one more commit I was about to push when I saw your approval: two read more links for |
7bc1f53
into
withastro:stable-live-collections
Description (required)
Content Loader API:
contentwithhtmlContentin theArticleinterfaceconstructortakes an optionalcodebut we never assign it while we're using it latererrorintry...catchblock:errorin the catch statement is inferred asunknown, so Typescript complains when usingerror.message. One solution is to useerror: anyif we want to avoid Typescript errors. But HiDeoo suggested a better approach!LiveLoadertype parameters (see below for the why... my explanation is a bit long 😁 )LiveLoaderobject and type (based on a previous T&D).renderMarkdown()mention inLiveDataEntry.rendered: this is only available for build-time loaders. This is still in the RFC Unresolved questions.astro:content:getLiveCollection()/getLiveEntry()code snippetsLiveDataCollectionResultandLiveDataEntryResult(note: this is imported fromastro, notastro:content)TODO: check
astro:contentnew descriptions/code snippets, this is more of a draft at the moment.Regarding
LiveLoaderfilters/type parameters:In short: if you use Typescript and have filters, you must provide the types, don't forget them.
Now, the detailed explanation:
I built Astro v6 (instead of using the experimental flag, to be safe) and I used it in a minimal project to check our code snippets. If we want to show valid examples without Typescript errors (well, except for things not implemented like
fetchFromCMS), theLiveLoadertype is a bit more complex to handle than what we describe.When I copy/paste the "Example live loader" code snippet:
filterinloadCollectionis inferred asundefined, which is fine: no Typescript errors (unless the user has a typed function instead offetchFromCMS).filterinloadEntryis inferred asnever, which means Typescript complains aboutfilter.id(Property 'id' does not exist on type 'never'.ts(2339)).Which is a bit odd because of:
It should fallback to
{id: string}but it doesn't seem to work. Typescript still think it'snever.So, from my understanding, this means:
LiveLoader<Article>means the loader doesn't accept any filters and so, we need to use:LiveLoader<Article, Record<string, any>>if onlyloadEntry()accepts filters.LiveLoader<Article, never, Record<string, any>>if onlyloadCollection()accepts filters.LiveLoader<Article, Record<string, any>, Record<string, any>>if both accepts filters.I also updated the "Error handling in live loaders" code snippets because:
undefinedas type parameter (Type 'undefined' does not satisfy the constraint 'Record<string, any>'.ts(2344)).neverinstead, we can't showfilter(ie. the loader doesn't accept any filters)Record<string, any>or a custom one (e.g. imported alongsideMyData).Since the goal is to show error handling, I think we can use
neverhere and we don't need to showfilter.I think systematically displaying the filters types each time
filteris used will help people understand that they are required.Related issues & labels (optional)