Add markdown check for absolute URLs in links to previous posts#541
Add markdown check for absolute URLs in links to previous posts#541frankieroberto wants to merge 4 commits into
Conversation
Links to other posts on the site are to be encouraged! But ideally they'd be relative links instead of full URLs. This means they'll work when previewing the site locally or in the review apps. It will also be helpful in the (hopefully unlikely) case that the site moves to a different domain name in future. Unlikely the existing check, for this one we can leave a direct suggestion on the pull request that authors can accept with a single click.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
references to the URL elsewhere are ok.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
github-actions
Bot
dismissed
their stale review
No markdown issues found — all clear.
edwardhorsford
left a comment
edwardhorsford
left a comment
There was a problem hiding this comment.
The include syntax looks like a mistake. Even if valid, is there a cleaner way of testing? Could compile the search string up front then do lineContent.startsWith(...) or similar?
| path: currentFile, | ||
| line: lineNumber, | ||
| message: ABSOLUTE_URL_MESSAGE, | ||
| suggestion: lineContent.replaceAll(SITE_URL, '/') |
There was a problem hiding this comment.
Might this fail to work if they've not included the https:// bit?
There was a problem hiding this comment.
Good point.
@copilot update the code so that it also works if the https:// prefix has been dropped, or if http:// is used.
| } | ||
| // Added line containing an absolute URL to the published site | ||
| const lineContent = rawLine.slice(1) | ||
| if (lineContent.includes('](' + SITE_URL)) { |
There was a problem hiding this comment.
This syntax seems really weird. Is it even valid?
There was a problem hiding this comment.
I now realise this is string concatenation. But I find it really hard to follow / it didn't look valid. Better would be string literal, though better still some variable that's described with a comment or regex.
| if (/^# /.test(lines[i])) { | ||
| mistakes.push({ path: filePath, line: i + 1, message: H1_MESSAGE }) | ||
| } | ||
| if (lines[i].includes('](' + SITE_URL)) { |
There was a problem hiding this comment.
This syntax is hard to follow
There was a problem hiding this comment.
I now realise this is string concatenation. But I find it really hard to follow / it didn't look valid. Better would be string literal, though better still some variable that's described with a comment or regex.
There was a problem hiding this comment.
This also won't work if there's a space in the markdown - eg [ Link text ]( url ), differences in casing (unlikely), if they don't include https://, or if it's an html link.
| } | ||
| if (lines[i].includes('](' + SITE_URL)) { | ||
| mistakes.push({ path: filePath, line: i + 1, message: ABSOLUTE_URL_MESSAGE }) | ||
| } |
There was a problem hiding this comment.
I think this will fail to work if https:// is not included
|
Another option would be: lineContent.includes(`](${SITE_URL}`)Though I think this still would fail if there are spaces in the markdown after the |
3 participants
Links to other posts on the site are to be encouraged!
But ideally they'd be relative links instead of full URLs. This means they'll work when previewing the site locally or in the review apps.
It will also be helpful in the (hopefully unlikely) case that the site moves to a different domain name in future.
Unlikely the existing check, for this one we can leave a direct suggestion on the pull request that authors can accept with a single click.
Will prevent this in future: #542
Screenshot