unjs
fix: relative link replacement
Resolves #94
This changes the relative link replacement, only image asset URLs are replaced/prefixed with the raw Github URL, while relative markdown URLs are simply replaced/prefixed with a normal (absolute) link to the branch tree.
Code is a bit messier with my changes 😅 happy to receive feedback.
Sorry it looks like there may be an edge case where this fails, checking if it's cache..
Oh, I ran into a separate related issue, worried it was my changes that did it 😂
On https://unjs.io/packages/changelogen see the link in the highlighted text, the replacement function replaced the URL string instead of the URL itself, I'll try and fix that as well 💪.
~It seems like some images still don't render correctly, but I'm not entirely sure if it's caused by the URL replacement logic.~
~See https://unjs.io/packages/automd for example, the badges and the contributor graph are not shown, while they are on Github https://github.com/unjs/automd.~
~The PR should fix the referenced issue as well as the behavior described in my previous comment, I'll leave the image rendering issue for another time or person 😅~
I see now that removing those is intentional, so all good!
@BobbieGoede changes look good, I must have missed since in the previous PR since I was trying to fix just relative images.
@pi0 The only thing should we unit test of the helper function to cover most cases?
Unit test would be nice! (also good for later refactors to move to an AST aware transform)
I just added very basic unit tests for this function and changed the ci file to run the tests, coverage only checks utils folder though.
Unit test would be nice! (also good for later refactors to move to an AST aware transform)
I figured I would try my hand at a refactor using AST to replace the links as hinted at, I kept it on a separate branch here https://github.com/BobbieGoede/ungh/pull/3.
No pressure to use those changes, it's primarily for me to get more experience using ASTs 😄
@pi0 / @cpreston321
Pinging in case this has slipt under the radar, let me know if any changes or updates need to be made 🙏 If you do merge this, you probably want to merge #119 as well since ci will break without it (corepack issue, unrelated to this PR).
I don't have enough bandwidth to check on ungh PRs in the upcoming weeks (at least until Nitro v3 is out) Sorry
Understood! I'll see if I can help with issue triage across unjs/nitro sometime instead 💪