-
-
Notifications
You must be signed in to change notification settings - Fork 478
fix(ui): resolve root-relative README markdown links to repo blob URLs #2929
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
base: main
Are you sure you want to change the base?
Changes from all commits
be3fd49
8c7bbae
5e7897b
a0b738c
03ae304
e7c9284
734f947
5f0c3bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -249,6 +249,70 @@ describe('Markdown File URL Resolution', () => { | |||||||||||||||||||
| 'href="https://github.com/test-owner/test-repo/blob/HEAD/CONTRIBUTING.md"', | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| }) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('resolves root-relative .md links to the repository root blob URL', async () => { | ||||||||||||||||||||
| const repoInfo = createRepoInfo({ | ||||||||||||||||||||
| directory: 'packages/core', | ||||||||||||||||||||
| }) | ||||||||||||||||||||
| const markdown = `[Root Contributing](/CONTRIBUTING.md)` | ||||||||||||||||||||
| const result = await renderReadmeHtml(markdown, 'test-pkg', repoInfo) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| expect(result.html).toContain( | ||||||||||||||||||||
| 'href="https://github.com/test-owner/test-repo/blob/HEAD/CONTRIBUTING.md"', | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| }) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('resolves root-relative .md links in raw HTML anchors', async () => { | ||||||||||||||||||||
| const repoInfo = createRepoInfo() | ||||||||||||||||||||
| const markdown = `<a href="/CONTRIBUTING.md">Contributing</a>` | ||||||||||||||||||||
| const result = await renderReadmeHtml(markdown, 'test-pkg', repoInfo) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| expect(result.html).toContain( | ||||||||||||||||||||
| 'href="https://github.com/test-owner/test-repo/blob/HEAD/CONTRIBUTING.md"', | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| }) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('resolves root-relative non-.md links to the repository root raw URL', async () => { | ||||||||||||||||||||
| const repoInfo = createRepoInfo({ | ||||||||||||||||||||
| directory: 'packages/core', | ||||||||||||||||||||
| }) | ||||||||||||||||||||
| const markdown = `[Logo](/assets/logo.png)` | ||||||||||||||||||||
| const result = await renderReadmeHtml(markdown, 'test-pkg', repoInfo) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| expect(result.html).toContain( | ||||||||||||||||||||
| 'href="https://raw.githubusercontent.com/test-owner/test-repo/HEAD/assets/logo.png"', | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| }) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('resolves authored root-relative npmx-like paths to the repository root raw URL', async () => { | ||||||||||||||||||||
| const repoInfo = createRepoInfo() | ||||||||||||||||||||
| const markdown = `[Package](/package/test-pkg)` | ||||||||||||||||||||
| const result = await renderReadmeHtml(markdown, 'test-pkg', repoInfo) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| expect(result.html).toContain( | ||||||||||||||||||||
| 'href="https://raw.githubusercontent.com/test-owner/test-repo/HEAD/package/test-pkg"', | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| }) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('keeps npmjs redirects local when repository info is available', async () => { | ||||||||||||||||||||
| const repoInfo = createRepoInfo() | ||||||||||||||||||||
| const markdown = `[Package](https://www.npmjs.com/package/test-pkg)` | ||||||||||||||||||||
| const result = await renderReadmeHtml(markdown, 'test-pkg', repoInfo) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| expect(result.html).toContain('href="/package/test-pkg"') | ||||||||||||||||||||
| }) | ||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason why we want this behaviour for non-markdown links? Why not something similar to what is done for this? npmx.dev/test/unit/server/utils/readme.spec.ts Lines 219 to 227 in 46e7c59
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. I updated this to resolve root-relative non-markdown links via Local npmx routes (
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason / place where preserving Local npmx routes would be useful instead of them resolving to rawBaseUrl too?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes. the main case is npmjs links that the README renderer intentionally converts to local npmx routes. For example, If we treated every root-relative path as a repo file, that converted route would incorrectly become So the updated logic preserves known npmx routes separately, while root-relative repo files like
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, is there no way to differentiate a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point. i will update this to preserve npmjs-originated links via an internal marker rather than path matching. README-authored root-relative paths now resolve normally against the repository. Added regression tests for both cases. |
||||||||||||||||||||
|
|
||||||||||||||||||||
| it('keeps npmjs route roots local when repository info is available', async () => { | ||||||||||||||||||||
| const repoInfo = createRepoInfo() | ||||||||||||||||||||
| const markdown = [ | ||||||||||||||||||||
| `[Packages](https://www.npmjs.com/package)`, | ||||||||||||||||||||
| `[Organizations](https://www.npmjs.com/org)`, | ||||||||||||||||||||
| ].join('\n') | ||||||||||||||||||||
| const result = await renderReadmeHtml(markdown, 'test-pkg', repoInfo) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| expect(result.html).toContain('href="/package"') | ||||||||||||||||||||
| expect(result.html).toContain('href="/org"') | ||||||||||||||||||||
| }) | ||||||||||||||||||||
| }) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| describe('without repository info', () => { | ||||||||||||||||||||
|
|
@@ -284,6 +348,14 @@ describe('Markdown File URL Resolution', () => { | |||||||||||||||||||
|
|
||||||||||||||||||||
| expect(result.html).toContain('href="https://docs.example.com/"') | ||||||||||||||||||||
| }) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('leaves protocol-relative URLs unchanged with repository info', async () => { | ||||||||||||||||||||
| const repoInfo = createRepoInfo() | ||||||||||||||||||||
| const markdown = `[CDN](//cdn.example.com/file.css)` | ||||||||||||||||||||
| const result = await renderReadmeHtml(markdown, 'test-pkg', repoInfo) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| expect(result.html).toContain('href="//cdn.example.com/file.css"') | ||||||||||||||||||||
| }) | ||||||||||||||||||||
| }) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| describe('anchor links', () => { | ||||||||||||||||||||
|
|
||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this because
resolveUrlis being called on the same link twice?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think it is, for changelog I prefix relative links for npmx with
$to prevent the resolveUrl/processUrl turning them into links for the git repo