-
Notifications
You must be signed in to change notification settings - Fork 4
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
bug: basePath
option name should align with corresponding Astro base
option name
#46
Comments
Technically, you're 100% correct that interface ConfigOptions {
contentPath: string;
collectionPathMode: 'subdirectory' | 'root';
astroConfig: {
base: string;
trailingSlash: 'always' | 'never' | 'ignore'
}
} I think this would help us divide the options and make it more clear which ones are based on Astro and which ones are specific to the plugin. |
I think namespacing could make a lot of sense depending on the direction of #45 and #47/#53/#27 in corresponding PR's #49/#50/#52. Especially with #45, given this is an Astro specific solution, the more I've thought about it, I think it makes sense to make this plugin a full Astro integration rather than just a rehype plugin as the only time to use it would be with Astro and if we move to an integration, we significantly reduce the options we need and can grab the Astro settings from the Astro config without exposing those settings integration wise and they become internal settings to the rehype plugin only (which the integration would be the only thing interacting with). Short of that, I think namespacing or not both have some merit. From reviewing other plugins, including Astro's own integrations, they don't tend to namespace when the integration has an option named identically to Astro's own option (e.g., Astro Rss). The other question is what happens when we have an option that is one of Astro's and we want to expose an override at the collection level for the same option. Following the approach of #47, it would look something like this I guess assuming namespace'd options: interface CollectionConfig {
astroOptionThatSupportsACollectionOverride: string
}
interface Options {
collections: Record<string, CollectionConfig>
astroConfig: {
astroOptionThatSupportsACollectionOverride: string
} Of course in the above, we would not use the exact same name in the Lol, I think I'm just rambling here because I'm torn. For me, it comes down to decision on #45 and if we do the integration, the namespacing question likely becomes moot. If we don't do #45 (or even if we do I guess), I'd likely lean namespace'd but only a small lean I think. |
I'll add that regardless of which way we go, would still recommend merging #48 now since assuming #47 is merged, we're going to have |
Thanks for the detailed response. I think it does make sense to keep it how it is (not namespaces) based on your examples and references to astro's own plugins... will merge soon |
fix: `basePath` option name should align with Astro `base` option name (#46)
Released in v0.15.0. Thanks! |
Where applicable, options & option names should align with its corresponding Astro option.
The basePath option is intended to correspond with Astro's base option and should be named accordingly.
The text was updated successfully, but these errors were encountered: