-
-
Notifications
You must be signed in to change notification settings - Fork 578
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
Add clientOptionsModule option to DocSearch plugin #2822
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 090f60f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
✅ Deploy Preview for astro-starlight ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Not a complete review or request for some changes, but I left a few comments mostly regarding the JSDoc to start some discussion around the approach.
One other question coming to mind is if we should expose a defineDocSearchConfig
helper just for the typings or something similar?
packages/docsearch/index.ts
Outdated
@@ -36,6 +38,38 @@ const DocSearchConfigSchema = z | |||
* @see https://www.algolia.com/doc/api-reference/search-api-parameters/ | |||
*/ | |||
searchParameters: z.custom<SearchOptions>(), | |||
/** | |||
* A JavaScript module containing a default export. These options |
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.
I think this should specify what the default export should be.
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.
Looking good @KianNH! Seconding @HiDeoo’s idea to make the option module all-or-nothing, so if you use that, you move all config there, unless there’s a reason not to do that anyone can see.
I also agree we should export something to help with typing the options module. Given this module will end up in client-side code, I might suggest just a type rather than a defineClientOptions()
utility just to avoid the extra no-op function in the bundle. (Would also allow us to export it directly from @astrojs/starlight-docsearch
rather than needing a dedicated file just for that.) That would help us assert that e.g. you shouldn’t set the container
property which Starlight takes care of.
So something like:
import type docsearch from '@docsearch/js';
export type DocSearchClientOptions = Omit<Parameters<typeof docsearch>[0], 'container'>;
And then users can write config as:
import type { DocSearchClientOptions } from '@astrojs/starlight-docsearch';
export default {
// ...
} satisfies DocSearchClientOptions;
Description
clientOptionsModule
option to thestarlightDocSearch
plugin.starlight/docsearch-config
virtual module.appId
, the option in theclientOptionsModule
file will be used.Considerations
starlight/docs/src/content/docs/guides/site-search.mdx
Lines 111 to 118 in 66a4131
Whilst this is probably not a large enough package to warrant it, I did setup try out setting up Vitest for this package where
vitePluginDocSearch
is passed invitest.config.ts
but since it would need a lot of work to setup a harness to teststarlightDocSearch
I opted against including it.