Skip to content

fix: relative link replacement#95

Open
BobbieGoede wants to merge 8 commits into
unjs:mainfrom
BobbieGoede:fix/relative-link-replacement
Open

fix: relative link replacement#95
BobbieGoede wants to merge 8 commits into
unjs:mainfrom
BobbieGoede:fix/relative-link-replacement

Conversation

@BobbieGoede

Copy link
Copy Markdown

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.

@BobbieGoede

Copy link
Copy Markdown
Author

Sorry it looks like there may be an edge case where this fails, checking if it's cache..

@BobbieGoede

Copy link
Copy Markdown
Author

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 💪.

@BobbieGoede BobbieGoede marked this pull request as ready for review July 16, 2024 20:03
@BobbieGoede

BobbieGoede commented Jul 16, 2024

Copy link
Copy Markdown
Author

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!

Comment thread utils/markdown.ts Outdated
Comment thread routes/repos/[owner]/[repo]/readme.ts
@cpreston321

Copy link
Copy Markdown
Member

@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?

@pi0

pi0 commented Jul 17, 2024

Copy link
Copy Markdown
Member

Unit test would be nice! (also good for later refactors to move to an AST aware transform)

@BobbieGoede

Copy link
Copy Markdown
Author

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.

@cpreston321

Copy link
Copy Markdown
Member

Thanks @BobbieGoede looks great.

@BobbieGoede

Copy link
Copy Markdown
Author

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 BobbieGoede#3.

No pressure to use those changes, it's primarily for me to get more experience using ASTs 😄

@BobbieGoede BobbieGoede force-pushed the fix/relative-link-replacement branch from 8428228 to 63b0117 Compare September 12, 2024 22:15
@BobbieGoede BobbieGoede force-pushed the fix/relative-link-replacement branch from 63b0117 to b551b85 Compare March 10, 2025 09:48
@BobbieGoede

Copy link
Copy Markdown
Author

@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).

@pi0

pi0 commented Apr 10, 2025

Copy link
Copy Markdown
Member

I don't have enough bandwidth to check on ungh PRs in the upcoming weeks (at least until Nitro v3 is out) Sorry

@BobbieGoede

Copy link
Copy Markdown
Author

Understood! I'll see if I can help with issue triage across unjs/nitro sometime instead 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Relative URL replacement breaks links to directories

3 participants