Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: relative link replacement #95

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

BobbieGoede
Copy link

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
Author

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

@BobbieGoede
Copy link
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
Copy link
Author

BobbieGoede commented Jul 16, 2024

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!

@cpreston321
Copy link
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
Copy link
Member

pi0 commented Jul 17, 2024

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

@BobbieGoede
Copy link
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
Member

Thanks @BobbieGoede looks great.

@BobbieGoede
Copy link
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
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