-
-
Notifications
You must be signed in to change notification settings - Fork 496
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 4 commits
be3fd49
8c7bbae
5e7897b
a0b738c
03ae304
e7c9284
734f947
5f0c3bb
d8e1f3d
972dbcc
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 |
|---|---|---|
|
|
@@ -269,6 +269,7 @@ const reservedPathsNpmJs = [ | |
| const npmJsHosts = new Set(['www.npmjs.com', 'npmjs.com', 'www.npmjs.org', 'npmjs.org']) | ||
|
|
||
| const USER_CONTENT_PREFIX = 'user-content-' | ||
| const LOCAL_NPMX_REDIRECT_PREFIX = '$npmx-local:' | ||
|
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 this because
Contributor
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. I do think it is, for changelog I prefix relative links for npmx with |
||
|
|
||
| function withUserContentPrefix(value: string): string { | ||
| return value.startsWith(USER_CONTENT_PREFIX) ? value : `${USER_CONTENT_PREFIX}${value}` | ||
|
|
@@ -315,6 +316,10 @@ export const isNpmJsUrlThatCanBeRedirected = (url: URL) => { | |
| return true | ||
| } | ||
|
|
||
| function toLocalNpmxRedirect(path: string): string { | ||
| return `${LOCAL_NPMX_REDIRECT_PREFIX}${path}` | ||
| } | ||
|
|
||
| /** | ||
| * Resolve a relative URL to an absolute URL. | ||
| * If repository info is available, resolve to provider's raw file URLs. | ||
|
|
@@ -323,6 +328,9 @@ export const isNpmJsUrlThatCanBeRedirected = (url: URL) => { | |
| */ | ||
| function resolveUrl(url: string, packageName: string, repoInfo?: RepositoryInfo): string { | ||
| if (!url) return url | ||
| if (url.startsWith(LOCAL_NPMX_REDIRECT_PREFIX)) { | ||
| return url.slice(LOCAL_NPMX_REDIRECT_PREFIX.length) | ||
| } | ||
| if (url.startsWith('#')) { | ||
| // Prefix anchor links to match heading IDs (avoids collision with page IDs) | ||
| // Normalize markdown-style heading fragments to the same slug format used | ||
|
|
@@ -338,15 +346,24 @@ function resolveUrl(url: string, packageName: string, repoInfo?: RepositoryInfo) | |
| const normalizedFragment = slugify(decodeHashFragment(fragment)) | ||
| return toUserContentHash(normalizedFragment || fragment) | ||
| } | ||
| // Absolute paths (e.g. /package/foo from a previous npmjs redirect) are already resolved | ||
| if (url.startsWith('/')) return url | ||
| // Check if this is a markdown file link | ||
| const isMarkdownFile = /\.md$/i.test(url.split('?')[0]?.split('#')[0] ?? '') | ||
|
|
||
| if (url.startsWith('/')) { | ||
| if (!repoInfo?.rawBaseUrl) { | ||
| return url | ||
| } | ||
|
|
||
| const baseUrl = isMarkdownFile ? repoInfo.blobBaseUrl : repoInfo.rawBaseUrl | ||
| return `${baseUrl}${url}` | ||
| } | ||
| if (hasProtocol(url, { acceptRelative: true })) { | ||
| try { | ||
| const parsed = new URL(url, 'https://example.com') | ||
| if (parsed.protocol === 'http:' || parsed.protocol === 'https:') { | ||
| // Redirect npmjs urls to ourself | ||
| if (isNpmJsUrlThatCanBeRedirected(parsed)) { | ||
| return parsed.pathname + parsed.search + parsed.hash | ||
| return toLocalNpmxRedirect(parsed.pathname + parsed.search + parsed.hash) | ||
| } | ||
| return url | ||
| } | ||
|
|
@@ -360,9 +377,6 @@ function resolveUrl(url: string, packageName: string, repoInfo?: RepositoryInfo) | |
| // for non-HTTP protocols (javascript:, data:, etc.), don't return, treat as relative | ||
| } | ||
|
|
||
| // Check if this is a markdown file link | ||
| const isMarkdownFile = /\.md$/i.test(url.split('?')[0]?.split('#')[0] ?? '') | ||
|
|
||
| // Use provider's URL base when repository info is available | ||
| // This handles assets that exist in the repo but not in the npm tarball | ||
| if (repoInfo?.rawBaseUrl) { | ||
|
|
||
| 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', () => { | ||||||||||||||||||||
|
|
||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.