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

bug: basePath option name should align with corresponding Astro base option name #46

Closed
techfg opened this issue Apr 21, 2024 · 5 comments · Fixed by #48
Closed

bug: basePath option name should align with corresponding Astro base option name #46

techfg opened this issue Apr 21, 2024 · 5 comments · Fixed by #48
Labels
breaking Indicates a breaking change, which the changelog generator should pick up appropriately

Comments

@techfg
Copy link
Contributor

techfg commented Apr 21, 2024

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.

@vernak2539
Copy link
Owner

Technically, you're 100% correct that basePath === base. I was thinking about this actually, and wondered if we should namespace the Astro specific config options. Something like below (excuse naming, but you will get the gist):

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.

@techfg
Copy link
Contributor Author

techfg commented May 3, 2024

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 CollectionConfig (e.g. astroOption in astroConfig and astroOptionOverride in CollectionConfig).

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.

@techfg
Copy link
Contributor Author

techfg commented May 3, 2024

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 srcDir and trailingSlash which are both identical names to Astro options so makes sense that base be identical until a potential larger refactor (e.g., namespace'd or integration).

@vernak2539
Copy link
Owner

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

@vernak2539 vernak2539 added the breaking Indicates a breaking change, which the changelog generator should pick up appropriately label May 16, 2024
vernak2539 added a commit that referenced this issue May 16, 2024
fix: `basePath` option name should align with Astro `base` option name (#46)
@vernak2539
Copy link
Owner

Released in v0.15.0. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Indicates a breaking change, which the changelog generator should pick up appropriately
Projects
None yet
2 participants