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

[WIP] Move route data to Astro.locals #2390

Draft
wants to merge 41 commits into
base: main
Choose a base branch
from
Draft

Conversation

delucis
Copy link
Member

@delucis delucis commented Sep 22, 2024

Description

  • Early draft PR exploring moving Starlight’s route data object to Astro.locals — currently this is passed down via Astro.props to all our templating components.

  • There are some tricky nuances here for sure.

  • For example, middleware runs for all routes on a site, which can include non Starlight pages. For these we can’t generate route data. For now I’ve reflected this in the types for locals and asserted Astro.locals.routeData! in the components. There’s not really a sensible way to guard that without duplicating a null check in every component, but also it feels like route data should be defined in all cases where Starlight’s <Page> is rendered.

  • The <StarlightPage> component poses some challenges. Right now you pass some props and it generates route data. I’ve hacked this in by assigning that generated data to locals inside the component for now, but this does mean you can’t transform data for pages created this way with additional middleware. Not sure there’s any way to solve this?

    • One way might be to have a dedicated system of “route data middleware” implemented at the Starlight level instead of using generic Astro middleware, e.g.

      starlight({
        routeMiddleware: './some-file.ts',
      })

      We could bundle those in a virtual module and have both Starlight’s locals.ts and <StarlightPage> use them or something:

      context.locals.routeData = await useRouteData(context);
      for (const fn of routeMiddleware) {
        context.locals.routeData = fn(context.locals.routeData);
      }
  • The current branch rips out the prop drilling entirely. That means any overrides that rely on Astro.props will break. Could it be worth doing something to ease migration? Deprecate props but keep them around? Throw an error something like we did for labels? Something to think about.

  • There’s probably a tidier way to do some of the code — just did the quick and easy thing for now. (For example, Content is currently typed as optional in the route data object to make it easy to throw it in where I needed it, without checking all the places that type is used. But could probably tidy that up to have a dedicated separate type.)

  • Can also discuss naming here. So far it’s Astro.locals.routeData, but there’s probably an argument for something a bit more descriptive like Astro.locals.starlightRoute or even just Astro.locals.starlight potentially.

To-do

  • Remove demo middleware from docs

Copy link

changeset-bot bot commented Sep 22, 2024

🦋 Changeset detected

Latest commit: 18faac2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@astrojs/starlight Minor

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

@github-actions github-actions bot added the 🌟 core Changes to Starlight’s main package label Sep 22, 2024
Copy link

netlify bot commented Sep 22, 2024

Deploy Preview for astro-starlight ready!

Name Link
🔨 Latest commit 18faac2
🔍 Latest deploy log https://app.netlify.com/sites/astro-starlight/deploys/6781c260c3ef1f00082f94de
😎 Deploy Preview https://deploy-preview-2390--astro-starlight.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 100 (no change from production)
Accessibility: 100 (no change from production)
Best Practices: 100 (no change from production)
SEO: 100 (no change from production)
PWA: -
View the detailed breakdown and full score reports

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

@astrobot-houston
Copy link
Collaborator

astrobot-houston commented Sep 22, 2024

size-limit report 📦

Path Size
/index.html 6.89 KB (0%)
/_astro/*.js 21.19 KB (0%)
/_astro/*.css 13.76 KB (0%)

@delucis delucis added the 🌟 minor Change that triggers a minor release label Sep 22, 2024
@lorenzolewis
Copy link
Contributor

Really exciting to see this!

The current branch rips out the prop drilling entirely. That means any overrides that rely on Astro.props will break. Could it be worth doing something to ease migration? Deprecate props but keep them around? Throw an error something like we did for labels? Something to think about.

With the i18n update didn't we have a somewhat breaking change (possibly that's the labels update you're talking about)? I feel like if it's at all possible to keep current behavior around and mark it as depreciated then it would be much more helpful. This change will probably impact A LOT of users who have spent time crafting component overrides.

Can also discuss naming here. So far it’s Astro.locals.routeData, but there’s probably an argument for something a bit more descriptive like Astro.locals.starlightRoute or even just Astro.locals.starlight potentially.

Is there any norm here in Astro-land for 3rd party integrations? This has a potential for name collisions if a user has implemented middleware, correct? If that's the case then I think it would be good to at the very least prefix any locals with starlight.

@HiDeoo
Copy link
Member

HiDeoo commented Sep 23, 2024

Sharing a few quick thoughts before I forget them as I've been playing with the idea locally too and we can discuss more in depth later.

For now I’ve reflected this in the types for locals and asserted Astro.locals.routeData! in the components.

As this impacts components and not user components, I've been experimenting with a getter function instead that throws if the route data is not available, which would mean it was called in a non-Starlight route.

The <StarlightPage> component poses some challenges.

Definitely a tricky one indeed, been trying a few things but haven't found something that I'm happy with yet too.

Deprecate props but keep them around? Throw an error something like we did for labels?

As we have seen from the 0.28.0 reports, it's relatively easy to craft explicit errors for Astro.props.* usage in user code altho it gets confusing for users when it's coming from a plugin.

If we compare to labels, in a panel of 15 plugins, only 3 where using Astro.props.labels so the impact was pretty low. This would definitely not be the case for all other props. Still need to think about this one but if we go with the same approach as labels, we should make sure to mention it in the changeset/breaking change that users should make sure their plugins are up to date (which is something I didn't think about for labels).

@delucis
Copy link
Member Author

delucis commented Dec 14, 2024

Spent a bit of time revisiting this PR and feeling quite positive about the idea. Made the following adjustments, which helps it feel better:

  1. I pulled 404 route data up into our route data process as well. This means that we can guarantee the locals will always contain Starlight data, even if it is only the 404 route for pages Starlight doesn’t know about. Avoids the need for a getter that throws an error (which I also played with for a bit) and means we really centralise all the data operations in the route data layer.

  2. I renamed locals.routeData to locals.starlightRoute. Feels better to namespace it a bit and also feels better when using things like entry.data where the repeat “data” in the path — locals.routeData.entry.data — felt kind of repetitive and vague. starlightRoute expresses things nicely I think.

There’s a type error I’m not 100% sure about — I guess we’re pulling some of our virtual modules into view for type checking that we weren’t previously? Not sure, but haven’t investigate too much. Next up I’d like to tackle the idea of a route data “middleware”/“pipeline” so that users and plugins have a place to plug in and modify stuff.

@github-actions github-actions bot added the 📚 docs Documentation website changes label Dec 18, 2024
@astrobot-houston
Copy link
Collaborator

astrobot-houston commented Dec 18, 2024

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

Locale File Note
en guides/overriding-components.mdx Source changed, localizations will be marked as outdated.
en guides/route-data.mdx Source added, will be tracked.
en reference/overrides.md Source changed, localizations will be marked as outdated.
en reference/route-data.mdx Source added, will be tracked.
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.

@delucis delucis added this to the v0.31 milestone Jan 9, 2025
@github-actions github-actions bot added the 🌟 docsearch Changes to Starlight’s DocSearch plugin label Jan 9, 2025
@delucis
Copy link
Member Author

delucis commented Jan 10, 2025

As this PR is going to impact plugin authors as well as regular Starlight users I wanted to give @HiDeoo, @martrapp, @Fevol, @trueberryless, and @lorenzolewis and extra heads up in case you have feedback.

You can read about how route data will be handled after this PR in the new “Route Data” guide in the preview deploy of our docs.

Basically anywhere you currently use Astro.props in component overrides will need updating to use Astro.locals.starlightRoute instead. Something that is new is the concept of “route middleware”, which will allow plugins to inject a function that can modify the data for the current route. So any plugin that does something like Astro.props.siteTitle = 'Some other title', filters the sidebar etc. will now be able to do that using a middleware function without necessarily injecting an override.

If any of you have thoughts on this change and how it impacts you, please let me know!

@delucis delucis modified the milestones: v0.31, v0.32 Jan 10, 2025
@trueberryless
Copy link
Contributor

I have really been looking forward to this PR for a some months, writing a lot with HiDeoo about how powerful these changes actually are. In my opinion, @HiDeoo describes it very well here (summed up from Discord):

Basically all the mutations these overrides do would be moved to a middleware, meaning the override would be left rendering something really small, e.g. a dropdown or something else, which can be exported to a component that the user can compose very easily in their own overrides. And for cases more complex, plugins will be able to also add things to route data, so another plugin could check for example Astro.locals.isBlogRoute === true or something like that and have some special handling for these very complex cases.

But at the same time, I think that the powerfulness of the Astro.locals is not demonstrated enough in the docs. In my humble opinion, adding a more complex example, like an override from Starlight Blog, to the docs would be awesome, alto I get the idea of documenting a very basic idea, like the ! added to all siteTitles...

Maybe, I'll just look at how HiDeoo will adapt the blog plugin and take it as a premium reference in order to write a short blog post on my own website about how overrides in Starlight are done the right and perfect way. I think that some documentation of that kind will help other plugin creators as well, but please let me know what you (Chris and plugin authors) think about my thoughts...

@trueberryless
Copy link
Contributor

Also, I have a small suggestion which I think should be discussed in the mentioned PR, not here...

@martrapp
Copy link
Member

Cool approach!

This might take a bit of time for people to migrate but doesn't seem too big a change and it is well documented.

Any ETA yet?

Shouldn't the examples include a call to next() at the end to enable possible chaining?

@trueberryless
Copy link
Contributor

Shouldn't the examples include a call to next() at the end to enable possible chaining?

I think as Chris mentioned in the description, that the for loop handles all middleware, so no next is needed:

for (const fn of routeMiddleware) {
  context.locals.routeData = fn(context.locals.routeData);
}

@martrapp
Copy link
Member

How breaking must a change be to justify a major version change? 😬

@HiDeoo
Copy link
Member

HiDeoo commented Jan 10, 2025

But at the same time, I think that the powerfulness of the Astro.locals is not demonstrated enough in the docs. In my humble opinion, adding a more complex example, like an override from Starlight Blog, to the docs would be awesome, alto I get the idea of documenting a very basic idea, like the ! added to all siteTitles...

Personally I think the Starlight feature is only context.locals.starlightRoute or Astro.locals.starlightRoute. The fact that it is attach to context.locals and Astro.locals is just a way to make it accessible to the user. The documentation links to the Astro documentation for the Astro render context which is the Astro feature used under the hood and I think the example you're suggesting is more of a plugin author taking advantage of the Astro feature rather than the new Starlight change in this case.

I guess a similar example could be a Starlight plugin adding an Astro integration to update the Astro config to add a remark plugin. That's something possible but the Starlight documentation does not have an example of that because it's mostly Astro features under the hood.

That's only my opinion of course but definitely open to discussing it further.

@trueberryless
Copy link
Contributor

How breaking must a change be to justify a major version change? 😬

I'm not entirely sure how the Starlight maintainers see this, but I think right now Starlight is not yet ready for v1 release... That's I think the reason why only minor releases happen. Once most of the issues are fixed and most users are satisfied, Starlight v1 will probably be released, but generally speaking that's a good question...

@trueberryless
Copy link
Contributor

I think the example you're suggesting is more of a plugin author taking advantage of the Astro feature rather than the new Starlight change in this case.

That's a very good point and 100% true. I meant exactly this advantage. This is the reason, why I came to the conclusion that writing my own blog to remind myself how I can use the features of Astro and Starlight in an exemplary way is probably the best option, leaving the current docs as they are. Thanks for the quick response!

@HiDeoo
Copy link
Member

HiDeoo commented Jan 10, 2025

I'm not entirely sure how the Starlight maintainers see this, but I think right now Starlight is not yet ready for v1 release...

There are definitely some planned features and fixes before a v1 release. Technically speaking about semver, "major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable." but I think we've been pretty good about only releasing breaking changes in minor versions with detailled changelog and migration guides.

@delucis
Copy link
Member Author

delucis commented Jan 10, 2025

Yeah, in general, we treat each Starlight minor like a small “major”. So no (intentional 😅) breaking changes in patches, but there can be breaking changes between minors as we iron out an API that we’ll keep for v1. That also aligns with how npm handles things, for example a dependency range of ^0.30.0 can install any 0.30.x patches, but won’t also include 0.31, unlike for later versions where ^1.30.0 would theoretically include all v1 releases, including e.g. 1.99.9.

Re: ETA, depends a bit — I have some details still to work on. But maybe end of next week is a plausible timeline.

@martrapp
Copy link
Member

Thanks, everyone, for the refreshers! I can explain it! ;-)

Most of the time I’ve worked with software, versioning usually started at 1.0. I'm still too used to thinking of 0.x versions as immature precursors that might never fully materialize. But that definitely doesn't apply to Starlight ;-) That's probably where my unease comes from. Apparently, I’m not the only one who finds years of 0.x versions odd. See Anthony Fu’s article on this for more: https://antfu.me/posts/epoch-semver.
Cheers!

@trueberryless
Copy link
Contributor

Quick comment: I love the documentation addition from a7eebad

Really nice example 👍

@martrapp
Copy link
Member

Really nice example 👍

First thing I'll do when it comes out, maybe a bit truncated ;-)

@trueberryless
Copy link
Contributor

@delucis Can we please ask Sarah if we can leave the middleware? 🥺

It looks awesome!!!

image

Or do we not even have to ask Sarah in the Starlight Docs, only in Astro i guess...

CHRIS CAN WE LEAVE THE MIDDLEWARE IN PLS??? 🥺 🥺 🥺 🥺 🤣

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌟 core Changes to Starlight’s main package 📚 docs Documentation website changes 🌟 docsearch Changes to Starlight’s DocSearch plugin 🌟 minor Change that triggers a minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants