fix: resolves #14, #15 & #16

[techfg]

1. Fixed tests that were validating incorrect behavior (e.g., index in root should remain the slug)
2. Eliminated duplicate parsing of url segments & querystringparams
3. Added test coverage for various scenarios
4. Resolves #14, Resolves #15, Resolves #16

If it's agreeable to you that this PR and #13 should be merged, would recommend merging this one first as it #13 contains more isolated changes compared to main.

Open to any/all feedback!

Potential Breaking Change

In PR #3 (based on issue #2), special case handling of index files was added. The fix in this PR related to #16, while I believe it to be correct based on this issue, will be a breaking change for anyone taking advantage of the current method of stripping for root collection index files only (subdirs in the collection will still be stripped as they are in Astro). Anyone needing a root collection index file to have index stripped from URL should apply a custom slug of empty string ('') as Astro requires.

[vernak2539]

If it's agreeable to you that this PR and https://github.com/vernak2539/astro-rehype-relative-markdown-links/pull/13 should be merged, would recommend merging this one first as it https://github.com/vernak2539/astro-rehype-relative-markdown-links/pull/13 contains more isolated changes compared to main.

Sorry! I didn't see this yet and now there's conflicts.

As an aside, is there a way to separate out the fixes for each issue into their own PR? Or, do you think since all the functionality you're submitting fixes for relates to each other they should remain in a singular PR?

[vernak2539]

@techfg do you have a link to this?

Anyone needing a root collection index file to have index stripped from URL should apply a custom slug of empty string ('') as Astro requires.

[techfg]

@techfg do you have a link to this?

Anyone needing a root collection index file to have index stripped from URL should apply a custom slug of empty string ('') as Astro requires.

@vernak2539 - Sure, it's here. I referenced that link in #16 and in the potential breaking changes section of this PR as well since it's rather important :)

[techfg]

@vernak2539 - You can also see that Astro only strips /index per this this line of code so root index slug would always remain index unless overridden via a slug.

To confirm this, you can go to the repro for #16 at https://stackblitz.com/edit/withastro-astro-7ge5gz. In the main site page, you'll see "All Pages" section (bottom) - this is Astro generating the links, not us since it's an astro file. If you inspect the links under otherstuff for Other Stuff Index and Other Stuff Subdir Index, you'll see /otherstuff/index and /otherstuff/subdir respectively. Astro automatically stripped index for the subdir but the collection root, which Astro treats as a site root, contains index for a slug.

[techfg]

If it's agreeable to you that this PR and #13 should be merged, would recommend merging this one first as it #13 contains more isolated changes compared to main.

Sorry! I didn't see this yet and now there's conflicts.

As an aside, is there a way to separate out the fixes for each issue into their own PR? Or, do you think since all the functionality you're submitting fixes for relates to each other they should remain in a singular PR?

No worries! Yeah, github had a heck of a time keeping the diff clean even though the changes themselves actually aren't that substantial.

The first 7 (of 8 total) commits to this PR are all tests, the 8th is the code changes. As I started working on #14, I found issue #15 which led to me finding #16.

As for separating 14/15/16, they are all very closely aligned since they all directly relate to how the transform url gets generated. The prior code did it in two different places (after #9 was added) so the first step was consolidating it back to one and then applying the logic for all three aspects. I can separate if you prefer but it might be more busy work for both of us than its worth. Completely up to you though, happy to go either way. Also happy to merge main in to this PR to save you the trouble.

Lemme know what you prefer, any direction you choose works for me :)

[vernak2539]

Sounds good keeping them in the same PR! May just take me a bit more time to parse, but that's a me problem.

Let me know once the conflicts are resolved, and I can have a look

[techfg]

@vernak2539 - main merged in to this PR. Fortunately, the actual changes in index.mjs are rather small so hopefully will be easy enough to parse :) Lemme know if you have any questions!

[vernak2539]

Also, after merging this, I'll be updating the async tests to be a different format, which may cause some conflicts in other PRs. Sorry, but I swear it will be better!

[techfg]

Hey @vernak2539 -

I added some comments, let me know if this is what you had in mind or would like something different. I'm glad you asked me to do this, found another bug (#25) while writing up the comments and also a potential feature (#24).

Re: breaking change - Agreed, don't think a major bump is needed given its stlil pre-release. That said, would recommend adding info about this to the README & CHANGELOG for anyone currently using library and just informational for anyone in the future since it's likely to come up since it doesn't appear Astro has any official documentation on how this is handled internally.

Re: tests - Funny you should mention, I thought about refactoring while I was in there but avoided the temptation lol A few things I considered but one was changing the way rehype is used to have a separate context for each test so they can run in parallel. Unfortunately, it doesn't appear that node test runner has anything like isolatemodulesasync unless there is something your aware of (my first time using node test runner). Would be ideal if you held off on this until after we finalize the remaining 3 PRs (the remaining 2 beyond this one are rather small and hopefully will be quick for you) but no worries if you want to do it now, happy to refactor to whatever approach you come up with!

[vernak2539]

Unfortunately, it doesn't appear that node test runner has anything like isolatemodulesasync unless there is something your aware of (my first time using node test runner).

Yeah, my first time using it as well. They run really fast already, so didn't even think about getting them to run in parallel

Would be ideal if you held off on this until after we finalize the remaining 3 PRs (the remaining 2 beyond this one are rather small and hopefully will be quick for you)

No worries! I can hold off.

Agreed, don't think a major bump is needed given its stlil pre-release. That said, would recommend adding info about this to the README & CHANGELOG

👍

[techfg]

@vernak2539 -

Thanks for merging, awesome!

re: tests - Not sure what IDE you use, but one thing I noticed in VSCode using the node test runner extension is that it doesn't detect the subtests. It's really a bug in the extension since subtests are completely valid but just FYI as you look to refactor tests since I'm not sure what you are thinking :) From what I could tell, you have the subtests in-place because of the lack of support for something similar to isolatemoduleasync since the tests can't run in parallel because of the way rehype uses the file history. Appreciate you holding off until after we decide on the PRs that remain, looking forward to your improvements!

re: readme - I saw the update you made, however I don't think it's completely accurate. The plugin still strips /index (a subdirectory index), it just doesn't strip index (root index) any more. The change made in #3 was correct with regards to subdirectory stripping, just not root stripping so the regex is on /index instead of just index. Recommend making that clear in the README if you think appropriate.

re: changelog - I saw the Fixed bullet under v.0.9.0 only cross-references #14, recommend including #15 & #16 if you think appropriate.

Thanks again!

[vernak2539]

re: tests - This is what I had in mind for them. It's really just using the describe correctly in the main file. I think I just went down the rabbit hole when trying to get them set up in the first place, going with anything that works.

I just use a regular terminal to run the tests, using yarn test --watch usually. It results in pretty quick tests (expand below). I'm not sure if that will fix VSCode extension, but it's a good start!

Test results

``` [update-tests] yarn test ▶ astroRehypeRelativeMarkdownLinks ✔ should transform file paths that exist (4.884083ms) ✔ should transform and contain index for root collection index.md file paths that exist (0.92075ms) ✔ should transform root collection index.md paths with empty string custom slug (1.80175ms) ✔ should transform root collection index.md paths with non-empty string custom slug (0.998167ms) ✔ should transform non-root collection index.md paths (0.796459ms) ✔ should transform encoded file paths that exist with capital letters (0.688125ms) ✔ should keep query and fragment for file paths that exist (0.4905ms) ✔ should prefix base to output on file paths that exist (0.387583ms) ✔ should not replace path if relative file does not exist (0.626083ms) ✔ should transform file paths that exist with non alphanumeric characters (0.868333ms) ✔ should transform with correct path when destination has custom slug (0.666ms) ✔ should transform with correct path when destination has custom slug with file extension (0.673334ms) ✔ should transform with correct path when destination in subpath has custom slug (0.517666ms) ✔ should not transform content collection path segment (0.523708ms) ✔ should not replace absolute path if file exists (0.253083ms) ✔ should not replace absolute path if file does not exist (0.223542ms) ✔ should not transform index.md file paths if file does not exist (0.21775ms) ✔ should not replace path if .md directory exists (0.327041ms) ✔ should not replace path if .mdx directory exists (0.215792ms) ✔ should not replace path if .md directory does not exist (0.159667ms) ✔ should not replace path if .mdx directory does not exist (0.139042ms) ▶ astroRehypeRelativeMarkdownLinks (18.001667ms) ▶ replaceExt ✔ replaces extension with another extension (1.522875ms) ✔ removes extension (0.058709ms) ✔ handles relative paths with single dot (0.050166ms) ✔ handles relative paths with two dots (0.1045ms) ✔ returns arg if not string (0.662625ms) ✔ returns path if length is zero (0.050959ms) ▶ replaceExt (4.342792ms) ▶ isValidRelativeLink ✔ return true if relative path to .md file (0.085833ms) ✔ return true if relative path to .mdx file (0.13025ms) ✔ return false if link empty string (0.0505ms) ✔ return false if link does not exist (0.097333ms) ✔ return false if not .md or .mdx file (0.032625ms) ✔ return false if an aboslute path (0.034417ms) ▶ isValidRelativeLink (0.790583ms) ▶ splitPathFromQueryAndFragment ✔ separates path with query string and hash (0.085167ms) ✔ separates path with query string only (0.094875ms) ✔ separates path with hash only (0.045125ms) ▶ splitPathFromQueryAndFragment (0.332584ms) ▶ generateSlug ✔ removes uppercase characters (0.225709ms) ✔ replaces spaces with dashes (0.054792ms) ✔ removes periods (0.155042ms) ✔ removes uppercase across multiple segments (0.040625ms) ✔ removes spaces across multiple segments with no leading slashes (0.033ms) ✔ should strip index when subdirectory (0.034958ms) ✔ should strip mixed-case index when subdirectory (0.031208ms) ✔ should not strip index root (0.030125ms) ✔ should not strip mixed case index root (0.0795ms) ▶ generateSlug (0.902042ms) ▶ resolveSlug ✔ uses generated slug when frontmatter undefined (0.937084ms) ✔ uses frontmatter when frontmatter empty string (0.051292ms) ✔ uses frontmatter when frontmatter is index (0.043292ms) ✔ uses frontmatter when frontmatter has extension (0.044458ms) ✔ throws exception when no valid slug (0.350542ms) ✔ throws exception when frontmatter is null (0.082166ms) ✔ throws exception when frontmatter is number (0.058833ms) ▶ resolveSlug (1.823708ms) ▶ normaliseAstroOutputPath ▶ prefix base to path ✔ base with no slashes (0.067084ms) ✔ base with slash at start (0.060208ms) ✔ base with slash at end (0.10025ms) ✔ base with slash at start and end (0.039417ms) ▶ prefix base to path (0.499125ms) ▶ normaliseAstroOutputPath (0.554333ms) ▶ isValidFile ✔ return true if relative path to .md file exists (0.24025ms) ✔ return true if relative path to .mdx file that exists (0.039625ms) ✔ return true if relative path to a file exists (0.036792ms) ✔ return false if relative path to .md file does not exist (0.10725ms) ✔ return false if relative path to .mdx file does not exist (0.097417ms) ✔ return false if relative path to a file does not exist (0.053958ms) ✔ return false if link empty string (0.030375ms) ✔ return false if link is null (0.026708ms) ✔ return false if link is undefined (0.026083ms) ✔ return false if path is a directory ending in .md that exists (0.0385ms) ✔ return false if path is a directory ending in .md/ that exists (0.033458ms) ✔ return false if path is a directory ending in .mdx that exists (0.035958ms) ✔ return false if path is a directory ending in .mdx/ that exists (0.031291ms) ✔ return false if path is a directory that exists (0.048ms) ▶ isValidFile (1.158625ms) ℹ tests 70 ℹ suites 9 ℹ pass 70 ℹ fail 0 ℹ cancelled 0 ℹ skipped 0 ℹ todo 0 ℹ duration_ms 179.360584 ```

re: changelog - Yeah, it's an interesting one as I don't have too much control over that due to the changelog being generated automatically based on commit messages. I can tinker a bit with it, but I'd rather not be regenerating the changelog and ignoring changes every time. I'll likely forget 😞 Resulted from me renaming the PR merge inaccurately I think.

re: readme - Will update to the content below. Thoughts?

In PR https://github.com/vernak2539/astro-rehype-relative-markdown-links/pull/3 (based on issue https://github.com/vernak2539/astro-rehype-relative-markdown-links/issues/2), special case handling of index files was added where the /index would be stripped from the URL. For example, src/content/collection/dir/index.md would be transformed into /collection/dir. This functionality was applied to content both at the root and in subdirectories. This functionality being applied to content at the root was removed in https://github.com/vernak2539/astro-rehype-relative-markdown-links/pull/17. To mimic this functionality, the slug frontmatter option provided by Astro can be used, defining an empty string ("").

[techfg]

re: tests - Yeah, I was just using the CLI, easy enough and was really fast which was impressive. For the refactor, I actually tried to the same (similar) approach you linked to but ran in to issues with rehype as it was seeming to share its state across each test because they were running in parallel that way vs. serially in the prior approach, this is why I was looking in to a solution for something similar to isolatedModuleAsync so that each test would get its own rehype module and not share state. Maybe I just structured them wrong - if your refactor works, awesome, much cleaner - just be careful and run the tests a lot to make sure there isn't any race/state conditions :)

re: changelog - Understood, only main downside to what's there now is that there is no way for a user to know that 14/15/16 were all fixed in v0.9.0 without looking at the PR directly (which most won't do 😦 ).

re: readme - Couple of minor suggestions:

In PR https://github.com/vernak2539/astro-rehype-relative-markdown-links/pull/3 (based on issue https://github.com/vernak2539/astro-rehype-relative-markdown-links/issues/2), special case handling of index files was added where the index would be stripped from the URL. For example, src/content/collection/dir/index.md would be transformed into /collection/dir. This functionality was applied to index.md files both at the content collection root and content collection subdirectories. In PR #17, applying this functionality to index.md at the collection root was removed based on the way Astro handles content at site/collection root vs. subdirectories (see this issue). If you want to have your collection root index.md be transformed without the index slug, utilitize the slug frontmatter option provided by Astro setting your slug to empty string ('').

[techfg]

Adding more clarity in last sentence:

In PR #17, applying this functionality to index.md at the collection root was removed based on the way Astro handles content at site/collection root vs. subdirectories (see this issue). If you want to have your collection root index.md be transformed without the index slug, utilitize the slug frontmatter option provided by Astro setting your slug to empty string ('') and ensure your getStaticPaths() returns undefined for the slug of the content collection root index.md.

[vernak2539]

Thanks for the content suggestions. README is updated!

[techfg]

@vernak2539 -

So it was bugging me that when the changelog generated for v0.9.0, it included the fix for 14 but not 15 & 16 which seemed odd since the commit message contained reference to all three of them as being resolved.

I looked in to auto-changelog (pretty cool tool) and it seems that its logic looks for any referenced commits inside the commit message, however, it's regex looks for individual mentions of issues, not ones separated by commas, etc. So, with the commit message I had .... Resolves #14, #15 & #16.... it only picked up 14.

Could go back and update the commit message and then it would pick it up when it re-gens but likely not worth the hassle as it would involve a rebase and then cause issues with anyone else that has the repo pulled so just FYI for future that whenever multiple issues are addressed in a single commit, ensure that they are fully qualified as resolved/closed/etc. (e.g., Resolved #14, Resolved #15, Resolved #16 or separate lines ,etc.)

Edit

Another option would be to manually add the fixes messages to the changelog, mark the existing changelog as locked as of above v0.9.0 (or any more recent version) and then update the auto-changelog config to start with a version more recent then where it was locked. Here's what it would like look if you're interested and even if not, good to know for future :)

package.json

  "auto-changelog": {
    "template": "keepachangelog",
    "hideCredit": true,
-    "commitLimit": false
+    "commitLimit": false,
+    "startingVersion": "v0.10.0"
  }

CHANGELOG.md

+ <!-- auto-changelog-above -->
## [v0.9.0](https://github.com/vernak2539/astro-rehype-relative-markdown-links/compare/v0.8.1...v0.9.0) - 2024-04-11

### Merged

 - fix: fixes url transformation and aligns to astro [`#17`](https://github.com/vernak2539/astro-rehype-relative-markdown-links/pull/17)
 - update to yarn v4.1.1 [`#23`](https://github.com/vernak2539/astro-rehype-relative-markdown-links/pull/23)

### Fixed

 - fix: fixes url transformation and aligns to astro (#17) [`#14`](https://github.com/vernak2539/astro-rehype-relative-markdown-links/issues/14)
+ - fix: fixes content collection name portion of path segment is slugified [`#15`](https://github.com/vernak2539/astro-rehype-relative-markdown-links/issues/15)
+ - fix: fixes incorrectly stripping index from content collection root index page [`#16`](https://github.com/vernak2539/astro-rehype-relative-markdown-links/issues/16)

### Commits

 - Release v0.9.0 [`a646e72`](https://github.com/vernak2539/astro-rehype-relative-markdown-links/commit/a646e729de6a2896f75b49cc621b0f4911adf8f7)
 - update changelog [`cff3ecf`](https://github.com/vernak2539/astro-rehype-relative-markdown-links/commit/cff3ecf11e6ced36af98c90ff4cd5c80bd32b2e8)

[vernak2539]

Thanks for this @techfg! I've ended up switching to https://github.com/github-changelog-generator/github-changelog-generator, which is much nicer and does all this stuff (even credit attribution... thought the other one did, but weirdly it didn't). so far, it's super nice. likely will try to generate the changelog automatically on merge of tags

[techfg]

Changelog looks good! Only downside is ruby is required so if anyone else needs to generate changelog, will take a few extra steps (e.g., install ruby, etc.) - possibly there is a way to automate that in a script or at least updating readme with information. Not needed for now though! In meantime, you can remove changelog script 😄

[vernak2539]

Only downside is ruby is required so if anyone else needs to generate changelog, will take a few extra steps (e.g., install ruby, etc.)

Yeah true, but it's a free github action due to it being public code! #winning

Not needed for now though! In meantime, you can remove changelog script 😄

Updated in #41! Good catch, thank you!