Skip to content

Component overrides feedback #861

@HiDeoo

Description

@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.

SCR-20231011-kwaa SCR-20231011-kwdg

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions