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

Initial Starlight Plugins support #942

Merged
merged 32 commits into from
Nov 29, 2023
Merged

Initial Starlight Plugins support #942

merged 32 commits into from
Nov 29, 2023

Conversation

HiDeoo
Copy link
Member

@HiDeoo HiDeoo commented Oct 23, 2023

What kind of changes does this PR include?

  • Changes or translations of Starlight docs site content
  • Changes to Starlight code

Description

This pull request is the beginning of the implementation of a Starlight Plugin API. A lot of details and discussions are available in #753 #962.

The following features are implemented in this first PR:

  • config
  • updateConfig()
  • addIntegration()
  • logger

I tried to comment various implementation details, specially around Zod, like why the plugin API is using a totally different schema than the configuration one, why the configuration and configuration updates flowing through the plugin API are not validated (but still properly typed for user convenience) and only the final configuration is validated, etc. but if you have any questions/suggestions, please let me know.

Plugin Example

This PR adds a temporary @astrojs/starlight-search-demo plugin that demonstrates the various features of the plugin API and see how it can be used.

This example is obviously a reference to a potential starlight-algolia kind of plugin that has been requested multiple times on Discord and would also be needed for the Astro docs site. The idea was to see the code required to implement such a plugin and having a custom search bar by only adding the following lines to the astro.config.mjs file:

import starlightSearchDemo from "@astrojs/starlight-search-demo";

starlight({
  plugins: [starlightSearchDemo({ apiKey: "123456" })],
});

Note that this plugin is only meant for demo purposes. It does nothing and even tho we briefly discussed the idea of having official plugins and if we ever decide to make an official Algolia plugin, it feels like this should be done in a separate PR or maybe even a separate repository with all expected features from such a plugin (proper CSS styling, pagefind disabled, tests, etc.).

Note: I think my personal important takeaway from this example is that the injectSettings() API discussed in #753 (and potentially in #861 too for some API changes) could be really useful.

Documentation

This PR adds a "Plugins Reference" page inspired by the Astro Integration API documentation page.
I'm not sure if a guide is needed for the plugin API at this point, as the "use a plugin" section would be pretty much a repeat of the plugins section of the Configuration Reference and may change between plugins and a "create a plugin" section, which I think does not exist for the Astro Integration API, would be pretty much merging the various examples from the "Plugins Reference" page.

I also suggested a change to the showcase to add a new section dedicated for plugins so it can be linked from various places. Obviously, we would need to wait for the first plugin to be published before doing that if we decide to go that way.

Remaining tasks

These tasks should be completed before merging this PR:

  • Remove the Starlight Search Demo plugin in its current state
  • Remove the Starlight Search Demo plugin from the docs configuration file
  • Remove all TODO(HiDeoo) references.
  • Add a changeset.

@changeset-bot
Copy link

changeset-bot bot commented Oct 23, 2023

🦋 Changeset detected

Latest commit: 1016921

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

@netlify

This comment was marked as outdated.

@github-actions github-actions bot added 📚 docs Documentation website changes 🌟 core Changes to Starlight’s main package labels Oct 23, 2023
@delucis
Copy link
Member

delucis commented Oct 23, 2023

Super exciting to see this! Will make sure to get you a thorough review ASAP.

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Super exciting work @HiDeoo! Left a few initial thoughts and questions.

docs/src/content/docs/reference/plugins.md Outdated Show resolved Hide resolved
packages/starlight/index.ts Outdated Show resolved Hide resolved
packages/starlight/utils/plugins.ts Outdated Show resolved Hide resolved
packages/starlight/utils/plugins.ts Outdated Show resolved Hide resolved
packages/starlight/utils/plugins.ts Outdated Show resolved Hide resolved
packages/starlight/utils/plugins.ts Outdated Show resolved Hide resolved
@HiDeoo
Copy link
Member Author

HiDeoo commented Nov 1, 2023

Thanks a lot for the initial feedback and discussions.

I think all of the points raised are now actionable and I can get back to it and prepare an updated version including all the suggested changes for further review 👍

* main: (103 commits)
  i18n(fr): update `guides/sidebar.mdx` (withastro#1033)
  i18n(fr): update `reference/configuration.mdx` (withastro#1034)
  i18n(fr): update `reference/frontmatter.md` (withastro#1035)
  Fix docs component details (withastro#1031)
  Overhaul getting started guide (withastro#1026)
  i18n(zh-cn): Update sidebar.mdx & configuration.mdx (withastro#1022)
  i18n(es): Update `configuration` & `sidebar` (withastro#1029)
  [ci] format
  i18n(zh-cn): Update frontmatter.md (withastro#1020)
  i18n(zh-cn): Update overrides.md (withastro#1021)
  i18n(zh-cn): Update i18n.mdx (withastro#1019)
  fix(docs-i18n-tracker): update `translations` import (withastro#1025)
  [ci] format
  i18n(zh-cn): Update css-and-tailwind.mdx (withastro#1018)
  [ci] format
  i18n(zh-cn): Update authoring-content.md (withastro#1016)
  i18n(ko-KR): update `configuration.mdx` (withastro#1015)
  i18n(ko-KR): update `sidebar.mdx` (withastro#1014)
  i18n(ko-KR): update `i18n.mdx` (withastro#1013)
  [ci] format
  ...
@HiDeoo
Copy link
Member Author

HiDeoo commented Nov 4, 2023

Add plugins to the user config schema

Turns out this one is a little bit "annoying" due to the recursive types (a user config including plugins which contains a function that pass down a user config that contains plugins…) and our tranforms.

I've setup a small repro in the TS playground of the issue. I've found so far 1 workaround that I describe in the playground but imo this is pretty ugly and I'm not sure if it's even worth it (considering we can do everything suggested without adding plugins to the user config I think).

I'll continue to explore this as I may be missing something obvious or a better way to do this but so far, no luck.

@HiDeoo
Copy link
Member Author

HiDeoo commented Nov 5, 2023

Just pushed a few changes relative to the first review:

  • Updated the shape of a plugin to match the one from an Astro integration.
  • The configuration passed down to a plugin now also contains the plugins list.
  • Refactor the entire validation process:
    1. Before running plugins, the user-provided configuration is validated (we can provided an early feedback to the user)
    2. Then, the user-provided plugins are validated (we can provided an early feedback to the user and indicate that a plugin is not valid)
    3. The plugins are executed
      3.1. If a plugin updates the configuration, we check that it does not try to mutate the plugins list (we have a dedicated error message indicating which plugin is responsible for the error)
      3.2. If a plugin updates the configuration, we validate the new configuration (if the merged configuration is not valid, we have a dedicated error message indicating which plugin is responsible for the error)
    4. We return the final validated configuration and the list of Astro integrations to add
  • I added a few comments to help understand the flow of the code regarding the validation process
  • A plugin now receives a few new options (astroConfig, command, and isRestart)
  • I updated the tests and documentation to reflect the changes
  • I slightly updated the demo
    • Fix the color contrast issue ^^
    • I added a button using Preact to show how a plugin should properly test if an Astro integration is already present before adding it

If I did not miss anything, this should covers all the feedback from the first review except the one about adding plugins directly to the user config schema. As commented right above, I still have not a found a proper way to do this that works as expected in all cases but I will keep looking into it.

@delucis delucis mentioned this pull request Nov 6, 2023
@delucis delucis added the 🌟 minor Change that triggers a minor release label Nov 17, 2023
@HiDeoo
Copy link
Member Author

HiDeoo commented Nov 28, 2023

Updated the PR with the following changes:

  • Remove demo plugin and its usage in the docs
  • Revert the showcase page to a pre-plugin version
  • Remove plugin showcase references from reference/configuration.mdx & reference/plugins.md
  • Search and remove all TODO(HiDeoo) references
  • Add changeset (not sure what level of detail is expected here)

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Really excited we’re getting so close!

I’ve left a last pass of comments on the docs & changeset, but then I think we should be good to go. 🚀

.changeset/lovely-keys-wash.md Outdated Show resolved Hide resolved
docs/src/content/docs/reference/plugins.md Show resolved Hide resolved
docs/src/content/docs/reference/plugins.md Outdated Show resolved Hide resolved
docs/src/content/docs/reference/plugins.md Outdated Show resolved Hide resolved
docs/src/content/docs/reference/plugins.md Outdated Show resolved Hide resolved
docs/src/content/docs/reference/plugins.md Outdated Show resolved Hide resolved
docs/src/content/docs/reference/plugins.md Outdated Show resolved Hide resolved
docs/src/content/docs/reference/plugins.md Outdated Show resolved Hide resolved
Co-authored-by: Chris Swithinbank <[email protected]>
@delucis
Copy link
Member

delucis commented Nov 29, 2023

Huh, not sure why CI started failing 🤔

Co-authored-by: Chris Swithinbank <[email protected]>
@delucis
Copy link
Member

delucis commented Nov 29, 2023

The CI failure is very odd — just ran the build with no issues locally.

@HiDeoo
Copy link
Member Author

HiDeoo commented Nov 29, 2023

The CI failure is very odd — just ran the build with no issues locally.

Same, working fine locally 🤔

@delucis
Copy link
Member

delucis commented Nov 29, 2023

From the stack trace it’s almost like it’s complaining about this:

if (!userConfig.pagefind) return;

But that’s not merged into this PR yet 🤔

@HiDeoo
Copy link
Member Author

HiDeoo commented Nov 29, 2023

Yeah I'm a bit confused as there is no userConfig anymore in the index.ts file anymore

@delucis
Copy link
Member

delucis commented Nov 29, 2023

Merged and fixed that to see if it helps!

@delucis
Copy link
Member

delucis commented Nov 29, 2023

Looks like that worked? I guess somehow there was some weird code in the CI job?

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

STAMP OF APPROVAL! ✅

@HiDeoo
Copy link
Member Author

HiDeoo commented Nov 29, 2023

Looks like that worked? I guess somehow there was some weird code in the CI job?

Yeah, I was afraid to try this, see it works and have no clue on why 🤣 I do not have a better explanation ^^

@delucis
Copy link
Member

delucis commented Nov 29, 2023

Well, here goes nothing… 🤞

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 🌟 minor Change that triggers a minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants