search

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
12 stars 4 forks source link

fix: handle collections in root of site (#18) #19

Closed techfg closed 6 months ago

techfg commented 6 months ago

Resolves #18

I based this PR off of PR #17 as it made solutioning for the issues covered in this one more straightforward to resolve than the prior code base would have. With that said, the solution I came up with does introduce a new option collectionPathMode.

Upon reviewing the potential approaches for addressing the issues, I just could not see a way to reliably and with certainty determine the collection path from the information we have available within the plugin. The prior code and even PR #17 and this PR still assume that a content collection is a subdirectory of the content path - normal behavior in Astro - but not always the case as #18 describes.

Given that, the only way to know for certain if the content collection and content path are the same is for the consumer of the library to tell us 😦

The default behavior (ContentPathMode = 'subdirectory') still applies and works identical to the way it did prior to this PR where we assume the content collections are immediate child directories of the content path. This PR simply allows for them to be the same on an opt-in basis. The same notes/warnings apply from those mentioned inAdditional Information` in the OP of #18.

vernak2539 commented 6 months ago

After the merge of #17 this PR now needs some merge conflicts resolved 😅

techfg commented 6 months ago

No worries :) I'll merge but we might not need this - see #27 for some thoughts and lemme know what you think.

techfg commented 6 months ago

Just pushed a merge but its not correct as I double up some tests - fixing now and will push again, sorry!

techfg commented 6 months ago

@vernak2539 - Ok, merge fixed, all set for you. FYI, similar to prior commits, I did not format the files to minimize diff alignment issues so they could likely use one :)

techfg commented 6 months ago

No worries :) I'll merge but we might not need this - see #27 for some thoughts and lemme know what you think.

Also, lemme know your thoughts on this and to a lesser but related degree your thoughts on #26, as depending on what they are, we might not need this PR.

vernak2539 commented 6 months ago

Interesting. I'll need a bit more time to parse and think about #26 and #27.

But, that doesn't mean we can't merge this one first and go from there. Will review now

techfg commented 6 months ago

Yeah, I'm still trying to think through all the scenarios across both issues lol

I think 27 has merit and might be a better path forward, more consistent with how Astro handles things internally. Still want to think it through some more.

For 26, I think we are going to run in to issues if we try to cross collection boundaries but there are benefits if the physical file structure matches the page path structure identically.

For this PR, even if we don't need collectionPathMode after deciding on 27, can always remove since we're pre-v1.

Look forward to your thoughts on 26 & 27 and lemme know if you have any questions on this PR in meantime.

Thanks!