Component overrides feedback

[HiDeoo]

This issue contains various early feedback regarding the new component overrides features after using it for various cases in various projects and integrations from overriding, extending and creating new routes.

Overriding and extending

This is the main feature/use case (hence the name ^^) and I think it works really well, very easy to use while still being very flexible. I managed to always be able to override what I needed and I guess there may be at some point some edge cases not found yet but this should be fairly easy to handle when they come up.

Types

I am adding this feedback in this issue but I am not yet sure yet this is directly related to component overrides.

Starting from v0.11.0, even tho everything looks fine in an editor like VSC, I can no longer type check my integrations using a basic tsc --noEmit command. For some reasons, it now tries to type check Starlight code in node_modules and report errors. I guess a potential reason could be the exports/types shenanigans we had to use to type components but I am not sure yet as I have used this syntax before without issues (but not with non-existing files so maybe that's the reason).

I will git bisect and play with traceResolution to figure it out, just didn't have the time yet.

Configuration

In a custom component, especially if shared between projects or coming from a third-party integration, it can be useful to read the Starlight configuration, e.g. to check if the editLink is enabled or not, get the default locale, etc.

Of course, we can import import starlightConfig from 'virtual:starlight/user-config' but this may be a bit of an unusual import for users, a bit of an implementation detail which is not documenated and we do not provide types for this. This means that for example in most of my integrations, I had to create a starlight.d.ts file to provide types for this import:

declare module "virtual:starlight/user-config" {
  const Config: import("@astrojs/starlight/types").StarlightConfig;

  export default Config;
}

This could also be done with the /// <reference path="../node_modules/@astrojs/starlight/virtual.d.ts" /> syntax but it relies on node_modules which is not ideal.

I am wondering if we should not export a function, e.g. getConfig or something like that maybe through @astrojs/starlight/config, which would be a bit more explicit and easier to type/use for this use case?

Props types

As Props depends on CollectionEntry<'docs'>, if you are using it in an integration for example that does not have type for this collection, you get basically no type infos for Astro.props.entry. This is a bit annoying as you basically either have to check the source code or see at runtime what you are missing. I am not sure tho there is a lot we can do about this one as the only solution would require some hard-coded types which is not ideal.

Creating new routes

I personally consider this to be an important use case, especially for integrations where creating markdown files is not an option like starlight-blog or starlight-openapi.

Route props

Consuming props and overriding some of them works well when overriding a component but I don't think it's the same case when creating a new route where you have to pass down the props yourself. I end up always having to create a helper function like this one for example:

export function getPageProps(title: string, slug: string): Props {
  const entryMeta = getEntryMeta();

  return {
    ...entryMeta,
    editUrl: undefined,
    entry: {
      data: {
        head: [],
        pagefind: false,
        title,
      },
      slug,
    },
    entryMeta,
    hasSidebar: true,
    headings: [],
    id: slug,
    lastUpdated: undefined,
    pagination: {
      next: undefined,
      prev: undefined,
    },
    sidebar: [],
    slug,
    toc: {
      items: [],
      maxHeadingLevel: 2,
      minHeadingLevel: 3,
    },
  };
}

I guess this is expected, but maybe not the best DX, you have most of the time to duplicate some infos like slug, etc.

The current architecture also makes it impossible to create a route using Starlight design but still being able to use the generated sidebar for example. In some cases, you use a custom sidebar that you generate on your new route (like starlight-blog for example for the blog posts page) but in some other cases like starlight-openapi, you want to use the default sidebar and only render some content in the page, which is now no longer possible.

I did not give it much thoughts yet but considering that <Page/> is not overridable (and I think this is good), I am wondering if we should not also remove the export of <Page/> and export a new <Route/> kinda component for this specific use-case. This one could maybe not accept the same Props as other components but by default use the default generated route data while still providing individual props to override them independently. For example, if I want to create a new route with Starlight design using the default generated route data, I could do something like this:

<Route {id} {title} {slug}>my content</Route>

And if I want to create a new route with Starlight design but with a custom sidebar, I could use something like that:

<Route {id} {title} {slug} hasSidebar sidebar={myCustomSidebar}>my content</Route>

Markdown styles

When you create a new route to render some custom content, some specific parts of it could be Markdown, e.g. a blog post excerpt in the blog posts list of starlight-blog or some specific schema fields accepting Markdown in starlight-openapi. You render this markdown on your side but most of the time, you want this markdown to get the same styles as Markdown in Starlight. You can obviously import <MarkdownContent/> and wrap your markdown with it but this component expects the classic Props. It's possible of course to pass down the page props/route data that you generated when creating the <Page/> but it's a bit of an annoying props drilling just to get some CSS.

I am wondering if having an additional exported component like <MarkdownStyles/> or something like that, breaking the "All components can access a standard Astro.props object that contains information about the current page." rule and not accepting anything and containing only CSS could be useful for this use case?

Table of contents

I am personally not a huge fan of the search bar / content shift between pages with a table of content and pages with no table of content.

If we take the case for example of starlight-blog, the blog posts list page does not need a table of contents while a blog post page has one and navigating between the two pages with the layout shift is a bit annoying imo.

Considering this, I do not pass down toc: undefined to avoid the layout shift but instead I pass down { items: [], maxHeadingLevel: 2, minHeadingLevel: 3 }. My expectation with this configuration would be an empty page sidebar but this is not the case, the "On this page" text is still rendered. I could override <TableOfContents/> but it feels weird to add an override just for this so I end up hiding it through CSS. Would it be possible that if the tableOfContents contains no items, we do not render anything?

toc vs headings documentation

The headings documentation says:

Use toc instead if you want to build a table of contents that respects Starlight’s configuration options.

It may be just me but I understood this as "if you want to build a table of contents, you can either use headings or toc" but this is not the case, toc is required to build a table of contents if I did not miss anything. I am wondering if we should not reword this sentence to make it clearer? Of course, this could also be totally me and I am the only one who understood it this way ^^

Props defaults

Not really an issue but more of an observation: with the usual pattern to use zod to provides some default and avoid checks/optional chaining in our components, to create a basic custom sidebar link for example, you have to do something like this:

{
  attrs: {},
  badge: undefined,
  href: '/slug',
  isCurrent: true,
  label: 'Link label',
  type: 'link',
}

The attrs: {} and badge: undefined are needed in this case. Like I said, not an issue or something that should be changed, just wanted to mention that it may become a bit annoying if at some point we have some objects with a lot of properties like this that are frequently used.

[HiDeoo]

Starting from v0.11.0, even tho everything looks fine in an editor like VSC, I can no longer type check my integrations using a basic tsc --noEmit command.

Ah turns out this is way less complicated that I imagined and not even related to this version.

Before component overrides, my integrations basically never imported anything from @astrojs/starlight directly but now this happens a lot, from components, Props, etc. meaning it is now part of what is being type checked but considering @astrojs/starlight itself fails a basic tsc --noEmit check, it fails in my integrations as well.

Would you be opened to a PR adding a tsc --noEmit check to @astrojs/starlight and fixing all the existing errors?

[delucis]

Would you be opened to a PR adding a tsc --noEmit check to @astrojs/starlight and fixing all the existing errors?

Definitely open! May not be 100% simple as I remember some things being hard to type-check (especially relating to astro:content), but would be great to get working if possible.

I am wondering if we should not export a function, e.g. getConfig or something

Would it make sense to add user config as a field to our RouteData object so you access it as part of Astro.props?

As Props depends on CollectionEntry<'docs'>, if you are using it in an integration for example that does not have type for this collection, you get basically no type infos for Astro.props.entry.

Ah yeah, that’s true. I do like depending on CollectionEntry<'docs'> because then if someone extends the schema, their custom fields show up here too. Maybe there’s a way to improve it with a conditional type?

type Entry =
  CollectionEntry<'docs'> extends unknown ? DefaultSchema : CollectionEntry<'docs'>

Not sure if that’s 100% what we’d want. Alternatively, there might be a way to generate a CollectionEntry equivalent that works for schemas missing content on-disk.

Creating new routes > Route props

I wonder if this is a case where something we abstract into a injectPages() plugin hook is how to solve this nicely? Not 100% sure what it would look like though given what you’re describing is using components directly. The most difficult thing would how to handle content.

Otherwise, yes, something like your <Route /> idea could work. I’d maybe bikeshed on it being called Route given it’s still just a component.

I am wondering if having an additional exported component like <MarkdownStyles/> […] containing only CSS could be useful for this use case?

A simpler version might be for us to extract and expose those styles in a content.css or markdown.css file you can import?

Would it be possible that if the tableOfContents contains no items, we do not render anything?

Perhaps! I’m wondering what the expected behaviour is for a page with no headings. In a regular page, you’d still get an Overview link to the top of the page. So I guess items: [] is basically impossible to hit in a regular Starlight page and this change would have no side effects.

toc vs headings documentation

It may be just me but I understood this as "if you want to build a table of contents, you can either use headings or toc" but this is not the case, toc is required to build a table of contents

I’m not sure I entirely follow. toc is in a format that’s more helpful for building a table of contents in Starlight’s default style (nested etc.), and already has items filtered out that should be excluded according to user config/frontmatter for this page.

headings is a flat array of all headings on a page. So you could build a table of contents with it if you wanted, or use it for some other purpose that requires knowing page headings.

Props defaults [resulting in verbose custom arguments]

Yeah, I’ve noticed this too. Not 100% sure what the right fix is. One thing that could help would be to make some helpers using the original schema?

[HiDeoo]

Thanks for the detailed response, really appreciate it! 🙌

Definitely open! May not be 100% simple as I remember some things being hard to type-check (especially relating to astro:content), but would be great to get working if possible.

Played a bit more with it, getting it correct in our repo with the current setup with a hoisted tsconfig.json is relatively easy but as soon as I tried linking my local repo in an integration, as the number of errors decreased a bit, I realized that I was getting more errors in my integration compared to my local repo. Turns out rules like noPropertyAccessFromIndexSignature were also applied even tho it's purely stylistic and not a type error.

After searching a bit more infos about it, and if I understand correctly this comment (and also this one) from a TS maintainer, this is due to Starlight shipping TS only (with no .d.ts) which I guess is not the best practice as this leads to this kind of issues.

Would it make sense to add user config as a field to our RouteData object so you access it as part of Astro.props?

I think this would work quite nicely altho before making this change, I would take into consideration the potential injectSettings() idea mentioned in #753 and #942 if we decide to implement it as this could potentially have some overlap.

Ah yeah, that’s true. I do like depending on CollectionEntry<'docs'> because then if someone extends the schema, their custom fields show up here too. Maybe there’s a way to improve it with a conditional type?

type Entry =
  CollectionEntry<'docs'> extends unknown ? DefaultSchema : CollectionEntry<'docs'>

Not sure if that’s 100% what we’d want. Alternatively, there might be a way to generate a CollectionEntry equivalent that works for schemas missing content on-disk.

Yeah, this is definitely something I can play with and see if I can come up with something that works but not something I would qualify as a super high priority.

I wonder if this is a case where something we abstract into a injectPages() plugin hook is how to solve this nicely? Not 100% sure what it would look like though given what you’re describing is using components directly. The most difficult thing would how to handle content.

Otherwise, yes, something like your <Route /> idea could work. I’d maybe bikeshed on it being called Route given it’s still just a component.

Yeah, this is a tricky one and I'm not sure what would be the best approach. I have been trying to play with some other ideas but didn't come up with anything that I really liked yet. I'll definitely keep thinking about it for a bit as this is for me my biggest pain point/"regression" for most of my integrations.

A simpler version might be for us to extract and expose those styles in a content.css or markdown.css file you can import?

Oh, I definitely like this idea way more 👍

Perhaps! I’m wondering what the expected behaviour is for a page with no headings. In a regular page, you’d still get an Overview link to the top of the page. So I guess items: [] is basically impossible to hit in a regular Starlight page and this change would have no side effects.

Yeah, I think you are correct and this would only impact someone explicitly overriding this prop and setting it to an empty array. I'll double check this when I have a bit more time.

I’m not sure I entirely follow.

I think I got the differences at the time but re-reading your explanation made me understand where my confusion was coming from. When the docs says "if you want to build a table of contents", it means building a custom ToC component rendering a table of contents using the data of the page. I understood this as (considering this is my primary use case) "if you want to render custom toc data using the built-in ToC component by providing your own data using either toc or headings".

[delucis]

this is due to Starlight shipping TS only (with no .d.ts) which I guess is not the best practice as this leads to this kind of issues

Ah yes, this was actually raised once on Discord by @loucyx. I was a bit resistant due to the added complexity of a build step, but if going forward this could be more of an issue, we could look at adding that.

When the docs says "if you want to build a table of contents", it means building a custom ToC component rendering a table of contents using the data of the page. I understood this as (considering this is my primary use case) "if you want to render custom toc data using the built-in ToC component by providing your own data using either toc or headings".

Ahhh, I see what you mean — confusion around what’s being built. Yeah, I meant a component. I guess could be easy to clarify just by sticking “component” on the end of the sentence.

---

Given there are quite a few issues here, it could be worth making new discussions at least for:

• compile .ts files and ship .js + .d.ts
• ship a reusable markdown.css
• improved custom page experience

[HiDeoo]

Given there are quite a few issues here, it could be worth making new discussions at least for:

Definitely, I'll tackle that tomorrow and also review the entire discussion to gather a few personal action items that do not require a new discussion to add to my personal todo list.

[HiDeoo]

I have opened the following discussions:

• Ship reusable Markdown styles #980
• Improve custom page injection #982
• Ship Starlight using `.js` + `.d.ts` files #983

I have noted various personal action items that do not require discussions and I will open PRs for them.

The only remaining concern is accessing config through RouteData which may require a bit more thinking but is tracked by #860, #753 and #942.

Considering the above, I feel confident that we can close this feedback issue.