Skip to content

fix(eslint-plugin): respect custom pageExtensions in no-html-link-for… #80035

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: canary
Choose a base branch
from
Open

fix(eslint-plugin): respect custom pageExtensions in no-html-link-for… #80035

wants to merge 2 commits into from
+296 −29

Conversation

viniciuspizettadesouza
Copy link

@viniciuspizettadesouza viniciuspizettadesouza commented Jun 1, 2025 •
edited
Loading

What?

Fixes a bug in the ESLint rule @next/next/no-html-link-for-pages, where it did not recognize internal pages using custom pageExtensions defined in next.config.js.

Why?

When users configure custom extensions (e.g., page.tsx, mdx), the rule still relied on a hardcoded default list (['js', 'jsx', 'ts', 'tsx']). This led to false positives by incorrectly flagging valid internal links as invalid.

This PR aligns the rule behavior with how Next.js resolves routes — respecting the developer’s intended page structure.

How?

  • Introduced a getPageExtensions() utility in eslint-plugin-next to dynamically load and normalize pageExtensions from next.config.js
  • Designed getPageExtensions() as a self-contained closure to:
    • Resolve and memoize config parsing once (avoiding repeated file system reads)
    • Cleanly encapsulate fallback logic and default handling
    • Allow internal utils like parseUrlForPages and parseUrlForAppDir to call it without receiving external params
  • Updated the ESLint rule to use this utility internally
  • Added unit tests to verify behavior when:
    • next.config.js is missing → uses default
    • Valid custom extensions → returned and applied
    • Invalid config (non-array) → safely falls back
  • Refactored getRegexPatterns() to receive page extensions from getPageExtensions(), ensuring:
    • ext and index patterns are correctly built from actual user config
    • Shared regex logic is cleanly encapsulated in a single place
    • All downstream consumers use a consistent pattern set

Benefits of the Closure Design

By using getPageExtensions() as a lazy, memoized closure:

  • There's no need to manually pass extensions through multiple internal layers
  • Consistency is guaranteed across all internal utility functions
  • We reduce boilerplate and cognitive overhead while improving testability
  • getRegexPatterns() now cleanly depends on a single canonical source for extensions

Additional Context

This change complements the fix in #68770, which resolved structural matching issues for App Router, but did not handle pageExtensions in pages/ linting.

It also supersedes partial or outdated approaches in:

Fixes

Fixes #53473

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jun 1, 2025 •
edited
Loading

⚠️ No Changeset found

Latest commit: d9e69bc

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@ijjk
Copy link
Member

ijjk commented Jun 1, 2025 •
edited
Loading

Allow CI Workflow Run

  • approve CI run for commit: d9e69bc

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

@viniciuspizettadesouza viniciuspizettadesouza force-pushed the fix/no-html-link-respect-page-extensions branch from fc0c19c to 007ebe0 Compare June 1, 2025 23:06
@darthmaim
Copy link
Contributor

This doesn't work with next.config.ts.

@viniciuspizettadesouza
Copy link
Author

This doesn't work with next.config.ts.

Good catch! You're right this currently only supports next.config.js. To support next.config.ts, we’d need either ts-node or a custom loader at runtime, which isn’t ideal in a linting context.

I can update the implementation to try both .js and .ts if ts-node is available, or add a fallback warning.

Let me know your thoughts

@viniciuspizettadesouza viniciuspizettadesouza force-pushed the fix/no-html-link-respect-page-extensions branch from b9fb27b to d9e69bc Compare June 1, 2025 23:53
@viniciuspizettadesouza viniciuspizettadesouza mentioned this pull request Jun 2, 2025
Open
1 task
@viniciuspizettadesouza
Copy link
Author

Hi @balazsorban44 @ijjk is there something missing to get a CI approval?

1 similar comment
@viniciuspizettadesouza
Copy link
Author

Hi @balazsorban44 @ijjk is there something missing to get a CI approval?

graphite-app[bot]
graphite-app bot reviewed Jun 13, 2025
View reviewed changes
packages/eslint-plugin-next/src/utils/url.ts
if (page.test(dirent.name)) {
res.push(`${urlprefix}${dirent.name.replace(page, '')}`)
} else if (!layout.test(dirent.name)) {
res.push(`${urlprefix}${dirent.name.replace(ext, '')}`)
}
} else {
const dirPath = path.join(directory, dirent.name)
if (dirent.isDirectory(dirPath) && !dirent.isSymbolicLink()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a small issue with the isDirectory() method call on line 57 of the parseUrlForAppDir function. The method doesn't accept any parameters, but it's being called with dirPath as an argument. This should be corrected to:

if (dirent.isDirectory() && !dirent.isSymbolicLink()) {

The isDirectory() method is a property of the Dirent object that returns a boolean indicating whether the entry is a directory, and it doesn't need any arguments to function correctly.

Suggested change
if (dirent.isDirectory(dirPath) && !dirent.isSymbolicLink()) {
if (dirent.isDirectory() && !dirent.isSymbolicLink()) {

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

@viniciuspizettadesouza
Copy link
Author

Hi @balazsorban44 @ijjk, is there anything missing on my side to get CI approval or move this PR (#80035) forward?

Thanks!

@alviarm alviarm mentioned this pull request Aug 8, 2025
Open
@alviarm
Copy link

alviarm commented Aug 8, 2025

Hi @viniciuspizettadesouza, I'm working on this issue as part of my open source contribution course assignment. I noticed the PR doesn't support next.config.ts as mentioned by @darthmaim. I've implemented this fix and created a PR to your branch. Would you be open to reviewing and merging it to help move your PR forward?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@graphite-app graphite-app[bot] graphite-app[bot] left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
tests type: next
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

@next/next/no-html-link-for-pages rule does not work with pageExtensions
4 participants
@viniciuspizettadesouza @ijjk @darthmaim @alviarm