vernak2539 / astro-rehype-relative-markdown-links

Rehype Plugin for Astro that allows for usage of relative links between markdown files
https://www.npmjs.com/package/astro-rehype-relative-markdown-links
15 stars 4 forks source link

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

Closed techfg closed 6 months ago

techfg commented 7 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

Released in v0.15.0. Thanks!