ElMassimo / iles

šŸ The joyful site generator
https://iles.pages.dev
MIT License
1.08k stars 31 forks source link

fix: anchor links in MDX files when prettyUrls is false #78

Closed GUI closed 2 years ago

GUI commented 2 years ago

Description šŸ“–

This attempts to fix a bug that appears to be present when prettyUrls: false which prevents MDX pages from linking to anchor tags on the page. Here is a reproduction of the issue: https://github.com/GUI/iles-debug-app/compare/anchor-links-pretty-urls

Background šŸ“œ

If the app is configured with prettyUrls: false, then anchor links in MDX files may not behave as expected. For example, a link like:

[foo](#bar)

Will be generated in the output as:

<a href="#bar.html">foo</a>

Instead of remaining href="#bar", like expected (since the anchor of '#bar.html` probably won't exist on the page).

Similarly, a link to /foo#bar will get translated to /foo#bar.html instead of what I think would be intended by how other URLs get rewritten (in which case, I think /foo.html#bar would be expected).

The Fix šŸ”Ø

This fixes the issue by skipping the MDX URL rewriting when anchor tags on the current page (no path is present) are encountered. For anchor links on other pages, the anchor is split from the path, and then the existing path rules are used, adding the anchor back in afterwards.

Using a URL parsing library, instead of simply splitting the string by # may be more robust, but Node.js's URL class doesn't handle relative links or fragment-only links, so it won't work for this use-case. But I think this simple splitting approach will hopefully meet the needs here.

But also welcome to any other suggestions for ways to fix this. Thanks!

nx-cloud[bot] commented 2 years ago

Nx Cloud Report

CI is running for commit a2f09ac536a3f77082cd086b4a6540d2e0bf20dd.

šŸ“‚ Click to track the progress, see the status, the terminal output, and the build insights.


Sent with šŸ’Œ from NxCloud.

ElMassimo commented 2 years ago

Hi Nick, thanks for the fix! šŸ˜ƒ

I've added a unit test for toExplicitHtmlPath to prevent regressions and ensure all cases are covered.


There's one case which wasn't working that still doesn't work after the fix:

expectExplicitPath('/about/nested.html').toEqual('/about/nested.html')

I'll make additional changes to cover this one before releasing a new version.

ElMassimo commented 2 years ago

Released in @islands/mdx 0.7.5.

GUI commented 2 years ago

Perfect, thanks again so much for the speedy fixes, this appears to be working great now. And thanks for the more complete and tidier fix and the test coverage (I hadn't found the test suite yet, but now I see in your commits).