-
Notifications
You must be signed in to change notification settings - Fork 23
Add ESLint rule to catch broken internal links #157
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: master
Are you sure you want to change the base?
Changes from all commits
ac4dcb6
6c856fb
42fa10b
6d00087
16e4757
60eae78
bccf53e
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 |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| node_modules/ | ||
| build/ | ||
| .docusaurus/ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| module.exports = { | ||
| parserOptions: { | ||
| ecmaVersion: 2022, | ||
| sourceType: "module", | ||
| ecmaFeatures: { jsx: true }, | ||
| }, | ||
| plugins: ["react"], | ||
| settings: { | ||
| react: { version: "detect" }, | ||
| }, | ||
| rules: { | ||
| "no-bad-internal-links": "error", | ||
| }, | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| name: Lint | ||
|
|
||
| on: | ||
| push: | ||
| branches: | ||
| - master | ||
| pull_request: | ||
| branches: | ||
| - master | ||
|
|
||
| jobs: | ||
| lint: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 20 | ||
| - name: Install dependencies | ||
| run: npm ci | ||
|
|
||
| - name: Lint | ||
| run: npm run lint | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| /** | ||
| * ESLint rule: no-bad-internal-links | ||
| * | ||
| * Enforces correct internal link format in JSX: | ||
| * - No leading/trailing whitespace in any URL | ||
| * - Internal links must start with '/' | ||
| * - Internal links must not use .html extension | ||
| * - Internal links must end with '/' | ||
|
Comment on lines
+6
to
+8
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. Are there technical reasons for all of these? I'm OK with requiring no extensions, but I like relative links and I think of a trailing slash as an indicator this page has children (eg |
||
| */ | ||
| module.exports = { | ||
| meta: { | ||
| type: "problem", | ||
| docs: { | ||
| description: | ||
| "Enforce correct internal link format in JSX (absolute paths, no .html, trailing slash)", | ||
| }, | ||
| messages: { | ||
| whitespace: | ||
| "Link has leading or trailing whitespace: '{{value}}'. Remove the extra spaces.", | ||
| noLeadingSlash: | ||
| "Internal link '{{value}}' should start with '/'. Did you mean '/{{suggested}}'?", | ||
| htmlExtension: | ||
| "Internal link '{{value}}' should not use .html extension. Did you mean '{{suggested}}'?", | ||
| noTrailingSlash: | ||
| "Internal link '{{value}}' should end with '/'. Did you mean '{{value}}/'?", | ||
| }, | ||
| }, | ||
| create(context) { | ||
| return { | ||
| JSXAttribute(node) { | ||
| const attrName = node.name && node.name.name; | ||
| if (attrName !== "href" && attrName !== "to") return; | ||
|
|
||
| // Only handle string literals, skip expressions like {`/path/${var}`} | ||
| if ( | ||
| !node.value || | ||
| node.value.type !== "Literal" || | ||
| typeof node.value.value !== "string" | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| const value = node.value.value; | ||
|
|
||
| // Check for leading/trailing whitespace (applies to all URLs) | ||
| if (value !== value.trim()) { | ||
| context.report({ node, messageId: "whitespace", data: { value } }); | ||
| return; | ||
| } | ||
|
|
||
| // Skip external URLs, anchors, and protocol-relative URLs | ||
| if ( | ||
| value.startsWith("http://") || | ||
| value.startsWith("https://") || | ||
| value.startsWith("mailto:") || | ||
| value.startsWith("tel:") || | ||
| value.startsWith("#") || | ||
| value.startsWith("//") | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| // Skip empty strings | ||
| if (value === "") return; | ||
|
|
||
| // --- Internal link checks --- | ||
|
|
||
| // Must start with / | ||
| if (!value.startsWith("/")) { | ||
| const stripped = value.replace(/\.html?$/, ""); | ||
| const suggested = stripped.endsWith("/") ? stripped : stripped + "/"; | ||
| context.report({ | ||
| node, | ||
| messageId: "noLeadingSlash", | ||
| data: { value, suggested }, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| // Must not end with .html | ||
| if (/\.html?$/.test(value)) { | ||
| const suggested = value.replace(/\.html?$/, "/"); | ||
| context.report({ | ||
| node, | ||
| messageId: "htmlExtension", | ||
| data: { value, suggested }, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| // Must end with / (except paths with anchors or query strings) | ||
| if ( | ||
| !value.endsWith("/") && | ||
| !value.includes("#") && | ||
| !value.includes("?") | ||
| ) { | ||
| context.report({ | ||
| node, | ||
| messageId: "noTrailingSlash", | ||
| data: { value }, | ||
| }); | ||
| } | ||
| }, | ||
| }; | ||
| }, | ||
| }; | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
Instead of creating a new file for linting check for CI, can we keep this in the existing
publish-docs.yaml? Basically, just add a new stepnpm run lintbeforenpm run build. Ideally, it should run beforenpm run build. wdyt?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 have no personal preference here, but i think it's better to have different CI flows for checks (it's what i have been doing in different codebases) but if you insist i can surely put it in existing yml file , let me know
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.
Then it’s fine as it is!