eslint(no-html-link-for-pages): respect custom pageExtensions #85295
Conversation
- Add `pageExtensions` parameter to rule options - URL parsing functions now accept pageExtensions array - Default to ['js','jsx','ts','tsx'] if not provided - Fixes vercel#53473: rule now detects .mdx, .md pages when configured Resolves: vercel#53473
|
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| if (url.includes('?')) { | ||
| url = url.split('?')[0] | ||
| } | ||
| url = url.split('?', 1)[0] | ||
| url = url.split('#', 1)[0] | ||
| url = url = url.replace(/(\/index\.html)$/, '/') | ||
| // Empty URLs should not be trailed with `/`, e.g. `#heading` | ||
| if (url === '') { | ||
| return url | ||
| } | ||
| url = url.endsWith('/') ? url : url + '/' | ||
| return url | ||
| } | ||
|
|
||
| /** | ||
| * Normalizes an app route so it represents the actual request path. Essentially | ||
| * performing the following transformations: | ||
| * | ||
| * - `/(dashboard)/user/[id]/page` to `/user/[id]` | ||
| * - `/(dashboard)/account/page` to `/account` | ||
| * - `/user/[id]/page` to `/user/[id]` | ||
| * - `/account/page` to `/account` | ||
| * - `/page` to `/` | ||
| * - `/(dashboard)/user/[id]/route` to `/user/[id]` | ||
| * - `/(dashboard)/account/route` to `/account` | ||
| * - `/user/[id]/route` to `/user/[id]` | ||
| * - `/account/route` to `/account` | ||
| * - `/route` to `/` | ||
| * - `/` to `/` | ||
| * | ||
| * @param route the app route to normalize | ||
| * @returns the normalized pathname | ||
| */ | ||
| export function normalizeAppPath(route: string) { | ||
| return ensureLeadingSlash( | ||
| route.split('/').reduce((pathname, segment, index, segments) => { | ||
| // Empty segments are ignored. | ||
| if (!segment) { | ||
| return pathname | ||
| } | ||
|
|
||
| // Groups are ignored. | ||
| if (isGroupSegment(segment)) { | ||
| return pathname | ||
| } | ||
|
|
||
| // Parallel segments are ignored. | ||
| if (segment[0] === '@') { | ||
| return pathname | ||
| } | ||
| const urlWithoutExtension = url.replace(/\.html$/, '') | ||
| // Encode all characters except `/` and `.` | ||
| const encoded = urlWithoutExtension | ||
| .split('/') | ||
| .map((segment) => encodeURIComponent(segment)) | ||
| .join('/') | ||
| .replace(/%2E/g, '.') | ||
|
|
||
| // The last segment (if it's a leaf) should be ignored. | ||
| if ( | ||
| (segment === 'page' || segment === 'route') && | ||
| index === segments.length - 1 | ||
| ) { | ||
| return pathname | ||
| } | ||
| const withoutTrailingSlash = | ||
| encoded === '/' ? '/' : encoded.replace(/\/$/, '') | ||
| return withoutTrailingSlash |
There was a problem hiding this comment.
The new normalizeURL implementation appears to be missing several behaviors from the original function:
- No handling for hash fragments (
url.split('#')[0]) - Different trailing slash behavior than the original implementation
- No special handling for empty URLs
- No null/undefined check before processing the URL
For compatibility with existing code, consider preserving these behaviors from the original implementation:
if (!url) {
return
}
url = url.split('?', 1)[0]
url = url.split('#', 1)[0]
// Empty URLs should not be trailed with `/`
if (url === '') {
return url
}The trailing slash behavior should also match the original function which added a trailing slash if not present.
| if (url.includes('?')) { | |
| url = url.split('?')[0] | |
| } | |
| url = url.split('?', 1)[0] | |
| url = url.split('#', 1)[0] | |
| url = url = url.replace(/(\/index\.html)$/, '/') | |
| // Empty URLs should not be trailed with `/`, e.g. `#heading` | |
| if (url === '') { | |
| return url | |
| } | |
| url = url.endsWith('/') ? url : url + '/' | |
| return url | |
| } | |
| /** | |
| * Normalizes an app route so it represents the actual request path. Essentially | |
| * performing the following transformations: | |
| * | |
| * - `/(dashboard)/user/[id]/page` to `/user/[id]` | |
| * - `/(dashboard)/account/page` to `/account` | |
| * - `/user/[id]/page` to `/user/[id]` | |
| * - `/account/page` to `/account` | |
| * - `/page` to `/` | |
| * - `/(dashboard)/user/[id]/route` to `/user/[id]` | |
| * - `/(dashboard)/account/route` to `/account` | |
| * - `/user/[id]/route` to `/user/[id]` | |
| * - `/account/route` to `/account` | |
| * - `/route` to `/` | |
| * - `/` to `/` | |
| * | |
| * @param route the app route to normalize | |
| * @returns the normalized pathname | |
| */ | |
| export function normalizeAppPath(route: string) { | |
| return ensureLeadingSlash( | |
| route.split('/').reduce((pathname, segment, index, segments) => { | |
| // Empty segments are ignored. | |
| if (!segment) { | |
| return pathname | |
| } | |
| // Groups are ignored. | |
| if (isGroupSegment(segment)) { | |
| return pathname | |
| } | |
| // Parallel segments are ignored. | |
| if (segment[0] === '@') { | |
| return pathname | |
| } | |
| const urlWithoutExtension = url.replace(/\.html$/, '') | |
| // Encode all characters except `/` and `.` | |
| const encoded = urlWithoutExtension | |
| .split('/') | |
| .map((segment) => encodeURIComponent(segment)) | |
| .join('/') | |
| .replace(/%2E/g, '.') | |
| // The last segment (if it's a leaf) should be ignored. | |
| if ( | |
| (segment === 'page' || segment === 'route') && | |
| index === segments.length - 1 | |
| ) { | |
| return pathname | |
| } | |
| const withoutTrailingSlash = | |
| encoded === '/' ? '/' : encoded.replace(/\/$/, '') | |
| return withoutTrailingSlash | |
| if (!url) { | |
| return | |
| } | |
| // Remove query parameters and hash fragments | |
| url = url.split('?', 1)[0] | |
| url = url.split('#', 1)[0] | |
| // Empty URLs should not be trailed with `/` | |
| if (url === '') { | |
| return url | |
| } | |
| const urlWithoutExtension = url.replace(/\.html$/, '') | |
| // Encode all characters except `/` and `.` | |
| const encoded = urlWithoutExtension | |
| .split('/') | |
| .map((segment) => encodeURIComponent(segment)) | |
| .join('/') | |
| .replace(/%2E/g, '.') | |
| // Ensure trailing slash behavior matches original implementation | |
| const withTrailingSlash = encoded.endsWith('/') ? encoded : `${encoded}/` | |
| return withTrailingSlash |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const pageUrls = cachedGetUrlFromPagesDirectories('/', foundPagesDirs, pageExtensions) | ||
| const appDirUrls = cachedGetUrlFromAppDirectory('/', foundAppDirs, pageExtensions) |
There was a problem hiding this comment.
The rule now forwards a pageExtensions variable to cachedGetUrlFromPagesDirectories and cachedGetUrlFromAppDirectory, but no variable with that name is declared in the function. When ESLint executes this rule the call will throw a ReferenceError, preventing the rule from running at all. You likely intended to resolve the extensions from options or context.settings before these calls.
Useful? React with 👍 / 👎.
| export function normalizeAppPath(path: string) { | ||
| const withoutTrailingSlash = path.replace(/\/$/, '') | ||
| return withoutTrailingSlash === '' ? '/' : withoutTrailingSlash |
There was a problem hiding this comment.
The new normalizeAppPath implementation only trims a trailing slash, whereas the previous version removed group segments like (dashboard), parallel segments (@foo), and the terminal page/route leaf. Because getUrlFromAppDirectory still relies on normalizeAppPath to normalize the paths collected from the app directory, URLs containing these segment types will now produce regexes that include the literal group names and won’t match real request paths (e.g. /user/[id] won’t match / (dashboard)/user/[id]/page). This causes the rule to miss links for most app-directory routes that use grouping.
Useful? React with 👍 / 👎.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| const pageUrls = cachedGetUrlFromPagesDirectories('/', foundPagesDirs, pageExtensions) | ||
| const appDirUrls = cachedGetUrlFromAppDirectory('/', foundAppDirs, pageExtensions) |
There was a problem hiding this comment.
The variable pageExtensions is used but never defined in the rule's create function. This will cause a ReferenceError at runtime.
View Details
📝 Patch Details
diff --git a/packages/eslint-plugin-next/src/rules/no-html-link-for-pages.ts b/packages/eslint-plugin-next/src/rules/no-html-link-for-pages.ts
index c522cda0ed..81a063ce12 100644
--- a/packages/eslint-plugin-next/src/rules/no-html-link-for-pages.ts
+++ b/packages/eslint-plugin-next/src/rules/no-html-link-for-pages.ts
@@ -73,6 +73,10 @@ export default defineRule({
const [customPagesDirectory] = ruleOptions
const rootDirs = getRootDirs(context)
+
+ const nextSettings: { pageExtensions?: string[] } =
+ context.settings.next || {}
+ const pageExtensions = nextSettings.pageExtensions
const pagesDirs = (
customPagesDirectory
Analysis
Missing variable definition in no-html-link-for-pages rule causes ReferenceError
What fails: Lines 110-111 in packages/eslint-plugin-next/src/rules/no-html-link-for-pages.ts reference undefined variable pageExtensions, causing TypeScript compilation error TS2304: "Cannot find name 'pageExtensions'".
How to reproduce:
cd packages/eslint-plugin-next
pnpm typesResult: TypeScript compiler error:
src/rules/no-html-link-for-pages.ts(110,76): error TS2304: Cannot find name 'pageExtensions'.
src/rules/no-html-link-for-pages.ts(111,72): error TS2304: Cannot find name 'pageExtensions'.
Root cause: Commit 7496bce updated the function signatures of getUrlFromPagesDirectories and getUrlFromAppDirectory to accept an optional pageExtensions parameter, but failed to extract and define pageExtensions in the rule's create function before using it on lines 110-111.
Fix: Extract pageExtensions from ESLint settings following the same pattern used by getRootDirs utility function, which reads from context.settings.next. Added extraction of pageExtensions from settings with proper TypeScript typing to match the expected optional string array parameter.
- Fix undefined resolvedPageExtensions variable in rule - Restore url.ts to upstream, apply minimal extension-only changes - All URL normalizers unchanged
|
Addressing linter feedback - pushed fixup commit 5d48967:
Ready for CI validation. |
| } | ||
| if (replaceMatch) { |
There was a problem hiding this comment.
| } | |
| if (replaceMatch) { | |
| } else if (replaceMatch) { |
The parseUrlForPages function incorrectly generates multiple URLs for index files, creating invalid routes like /index in addition to the correct / route.
View Details
Analysis
Bug: parseUrlForPages generates invalid /index routes for index files
What fails: The parseUrlForPages() function in packages/eslint-plugin-next/src/utils/url.ts (lines 29-37) generates both / and /index URLs for index files, when only / is a valid Next.js route.
How to reproduce:
// Create a pages directory with index.tsx file
parseUrlForPages('/', 'pages/', undefined)
// With a file named: pages/index.tsxResult: Returns ['/', '/index'] instead of just ['/']
Expected: Should return only ['/'] because in Next.js, pages/index.tsx creates a route at /, not /index. The /index route does not exist in Next.js.
Impact: The ESLint rule no-html-link-for-pages incorrectly flags <a href="/index"> as needing to use <Link> component, when /index is not an actual Next.js route. Additionally, it may miss flagging actual invalid links if the URL normalization deduplicates the patterns.
Root cause: Lines 32-37 use two independent if statements. For index files:
indexMatchcorrectly matches and creates/replaceMatchalso matches (since.tsxmatches/(\..+?)$/) and creates/index
Both conditions execute for index files, creating the invalid route. The second condition should only execute if NOT an index file (using else if).
Note: The parallel parseUrlForAppDir() function (lines 45-72) already implements the correct pattern using else if logic, confirming this is the intended behavior.
3 participants
Fixes
@next/next/no-html-link-for-pagesto respect custompageExtensions(incl..mdx/.md).Backwards compatible (defaults to
['js','jsx','ts','tsx']).Supports both rule option and
settings.next.pageExtensions.Why
With
pageExtensions: ['mdx','md','tsx'],<a href="/blog">wasn't flagged whenpages/blog.mdxexists because extensions were hardcoded in the ESLint rule.How
pageExtensionsoption; falls back tosettings.next.pageExtensionsChanges
packages/eslint-plugin-next/src/utils/url.ts— dynamic regex builderpackages/eslint-plugin-next/src/rules/no-html-link-for-pages.ts— option parsing + settings fallback.changeset/eslint-pageextensions-support.md— version bump (patch)Usage
Via Rule Option
{ "rules": { "@next/next/no-html-link-for-pages": ["warn", { "pageExtensions": ["mdx", "md", "tsx", "ts", "jsx", "js"] }] } }Via ESLint Settings
{ "settings": { "next": { "pageExtensions": ["mdx", "md", "tsx", "ts", "jsx", "js"] } } }Impact
.mdxpages now properly detected.mdpages now properly detectedFixes #53473