Skip to content
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

Fix linting of no-html-link in appDir #51783

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
fa30abd
Fix linting of no-html-link in appDir
DerTimonius Jun 25, 2023
31a7919
Merge branch 'canary' into fix/lint-html-pages-appdir
DerTimonius Jul 3, 2023
f6d5361
Merge branch 'canary' into fix/lint-html-pages-appdir
DerTimonius Jul 7, 2023
4d51254
Implement suggested normalizeAppPath function
DerTimonius Jul 8, 2023
205246b
Add test for intercepted routes
DerTimonius Jul 23, 2023
23f2bec
Merge branch 'canary' into fix/lint-html-pages-appdir
DerTimonius Jul 23, 2023
35f2957
Merge branch 'canary' into fix/lint-html-pages-appdir
shuding Jul 27, 2023
f697f77
Merge branch 'canary' into fix/lint-html-pages-appdir
ijjk Jul 29, 2023
4eac63d
Fix variables in parseUrlForAppDir
DerTimonius Jul 29, 2023
a841f96
Merge branch 'canary' into fix/lint-html-pages-appdir
DerTimonius Aug 7, 2023
a620753
Change variable to dirent
DerTimonius Aug 7, 2023
76f3589
Merge branch 'canary' into fix/lint-html-pages-appdir
DerTimonius Aug 14, 2023
d645679
Merge branch 'canary' into fix/lint-html-pages-appdir
DerTimonius Aug 21, 2023
8df4973
Merge branch 'canary' into fix/lint-html-pages-appdir
DerTimonius Aug 29, 2023
acb654b
Merge branch 'canary' into fix/lint-html-pages-appdir
DerTimonius Sep 13, 2023
1eb450e
Merge branch 'canary' into fix/lint-html-pages-appdir
DerTimonius Sep 20, 2023
75dc146
Merge branch 'canary' into fix/lint-html-pages-appdir
DerTimonius Oct 22, 2023
c52a00a
Merge branch 'canary' into fix/lint-html-pages-appdir
DerTimonius Nov 20, 2023
71da0cb
Merge branch 'canary' into fix/lint-html-pages-appdir
DerTimonius Dec 8, 2023
95bd20b
Merge branch 'canary' into fix/lint-html-pages-appdir
DerTimonius Jan 9, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
getUrlFromPagesDirectories,
normalizeURL,
execOnce,
getUrlFromAppDirectory,
} from '../utils/url'

const pagesDirWarning = execOnce((pagesDirs) => {
Expand Down Expand Up @@ -93,6 +94,8 @@ export = defineRule({
}

const pageUrls = getUrlFromPagesDirectories('/', foundPagesDirs)
const appDirUrls = getUrlFromAppDirectory('/', foundAppDirs)
const allUrls = [...pageUrls, ...appDirUrls]
return {
JSXOpeningElement(node) {
if (node.name.name !== 'a') {
Expand Down Expand Up @@ -134,8 +137,8 @@ export = defineRule({
return
}

pageUrls.forEach((pageUrl) => {
if (pageUrl.test(normalizeURL(hrefPath))) {
allUrls.forEach((foundUrl) => {
if (foundUrl.test(normalizeURL(hrefPath))) {
context.report({
node,
message: `Do not use an \`<a>\` element to navigate to \`${hrefPath}\`. Use \`<Link />\` from \`next/link\` instead. See: ${url}`,
Expand Down
49 changes: 49 additions & 0 deletions packages/eslint-plugin-next/src/utils/url.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as path from 'path'
import * as fs from 'fs'
import { normalizeAppPath } from '../../../next/src/shared/lib/router/utils/app-paths'

// Cache for fs.readdirSync lookup.
// Prevent multiple blocking IO requests that have already been calculated.
Expand Down Expand Up @@ -33,6 +34,33 @@ function parseUrlForPages(urlprefix: string, directory: string) {
return res
}

/**
* Recursively parse app directory for URLs.
*/
function parseUrlForAppDir(urlprefix: string, directory: string) {
fsReadDirSyncCache[directory] ??= fs.readdirSync(directory, {
withFileTypes: true,
})
const res = []
fsReadDirSyncCache[directory].forEach((dirent) => {
// TODO: this should account for all page extensions
// not just js(x) and ts(x)
if (/(\.(j|t)sx?)$/.test(dirent.name)) {
if (/^page(\.(j|t)sx?)$/.test(dirent.name)) {
res.push(`${urlprefix}${dirent.name.replace(/^page(\.(j|t)sx?)$/, '')}`)
} else if (!/^layout(\.(j|t)sx?)$/.test(dirent.name)) {
res.push(`${urlprefix}${dirent.name.replace(/(\.(j|t)sx?)$/, '')}`)
}
} else {
const dirPath = path.join(directory, dirent.name)
if (dirent.isDirectory(dirPath) && !dirent.isSymbolicLink()) {
res.push(...parseUrlForPages(urlprefix + dirent.name + '/', dirPath))
}
}
})
return res
}

/**
* Takes a URL and does the following things.
* - Replaces `index.html` with `/`
Expand Down Expand Up @@ -77,6 +105,27 @@ export function getUrlFromPagesDirectories(
})
}

export function getUrlFromAppDirectory(
urlPrefix: string,
directories: string[]
) {
return Array.from(
// De-duplicate similar pages across multiple directories.
new Set(
directories
.map((directory) => parseUrlForAppDir(urlPrefix, directory))
.flat()
.map(
// Since the URLs are normalized we add `^` and `$` to the RegExp to make sure they match exactly.
(url) => `^${normalizeAppPath(url)}$`
)
)
).map((urlReg) => {
urlReg = urlReg.replace(/\[.*\]/g, '((?!.+?\\..+?).*?)')
return new RegExp(urlReg)
})
}

export function execOnce<TArgs extends any[], TResult>(
fn: (...args: TArgs) => TResult
): (...args: TArgs) => TResult {
Expand Down
154 changes: 154 additions & 0 deletions test/unit/eslint-plugin-next/no-html-link-for-pages.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,36 @@ export class Blah extends Head {
}
`

const validInterceptedRouteCode = `
import Link from 'next/link';

export class Blah extends Head {
render() {
return (
<div>
<Link href='/photo/1/'>Photo</Link>
<h1>Hello title</h1>
</div>
);
}
}
`

const invalidInterceptedRouteCode = `
import Link from 'next/link';

export class Blah extends Head {
render() {
return (
<div>
<a href='/photo/1/'>Photo</a>
<h1>Hello title</h1>
</div>
);
}
}
`

describe('no-html-link-for-pages', function () {
it('prints warning when there are no "pages" or "app" directories', function () {
const consoleSpy = jest.spyOn(console, 'warn').mockImplementation()
Expand Down Expand Up @@ -370,4 +400,128 @@ describe('no-html-link-for-pages', function () {
'Do not use an `<a>` element to navigate to `/list/lorem-ipsum/`. Use `<Link />` from `next/link` instead. See: https://nextjs.org/docs/messages/no-html-link-for-pages'
)
})
it('valid link element with appDir', function () {
const report = withAppLinter.verify(validCode, linterConfig, {
filename: 'foo.js',
})
assert.deepEqual(report, [])
})

it('valid link element with multiple directories with appDir', function () {
const report = withAppLinter.verify(validCode, linterConfig, {
filename: 'foo.js',
})
assert.deepEqual(report, [])
})

it('valid anchor element with appDir', function () {
const report = withAppLinter.verify(validAnchorCode, linterConfig, {
filename: 'foo.js',
})
assert.deepEqual(report, [])
})

it('valid external link element with appDir', function () {
const report = withAppLinter.verify(validExternalLinkCode, linterConfig, {
filename: 'foo.js',
})
assert.deepEqual(report, [])
})

it('valid download link element with appDir', function () {
const report = withAppLinter.verify(validDownloadLinkCode, linterConfig, {
filename: 'foo.js',
})
assert.deepEqual(report, [])
})

it('valid target="_blank" link element with appDir', function () {
const report = withAppLinter.verify(
validTargetBlankLinkCode,
linterConfig,
{
filename: 'foo.js',
}
)
assert.deepEqual(report, [])
})

it('valid public file link element with appDir', function () {
const report = withAppLinter.verify(validPublicFile, linterConfig, {
filename: 'foo.js',
})
assert.deepEqual(report, [])
})

it('invalid static route with appDir', function () {
const [report] = withAppLinter.verify(invalidStaticCode, linterConfig, {
filename: 'foo.js',
})
assert.notEqual(report, undefined, 'No lint errors found.')
assert.equal(
report.message,
'Do not use an `<a>` element to navigate to `/`. Use `<Link />` from `next/link` instead. See: https://nextjs.org/docs/messages/no-html-link-for-pages'
)
})

it('invalid dynamic route with appDir', function () {
const [report] = withAppLinter.verify(invalidDynamicCode, linterConfig, {
filename: 'foo.js',
})
assert.notEqual(report, undefined, 'No lint errors found.')
assert.equal(
report.message,
'Do not use an `<a>` element to navigate to `/list/foo/bar/`. Use `<Link />` from `next/link` instead. See: https://nextjs.org/docs/messages/no-html-link-for-pages'
)
const [secondReport] = withAppLinter.verify(
secondInvalidDynamicCode,
linterConfig,
{
filename: 'foo.js',
}
)
assert.notEqual(secondReport, undefined, 'No lint errors found.')
assert.equal(
secondReport.message,
'Do not use an `<a>` element to navigate to `/list/foo/`. Use `<Link />` from `next/link` instead. See: https://nextjs.org/docs/messages/no-html-link-for-pages'
)
const [thirdReport] = withAppLinter.verify(
thirdInvalidDynamicCode,
linterConfig,
{
filename: 'foo.js',
}
)
assert.notEqual(thirdReport, undefined, 'No lint errors found.')
assert.equal(
thirdReport.message,
'Do not use an `<a>` element to navigate to `/list/lorem-ipsum/`. Use `<Link />` from `next/link` instead. See: https://nextjs.org/docs/messages/no-html-link-for-pages'
)
})

it('valid intercepted route with appDir', function () {
const report = withAppLinter.verify(
validInterceptedRouteCode,
linterConfig,
{
filename: 'foo.js',
}
)
assert.deepEqual(report, [])
})

it('invalid intercepted route with appDir', function () {
const [report] = withAppLinter.verify(
invalidInterceptedRouteCode,
linterConfig,
{
filename: 'foo.js',
}
)
assert.notEqual(report, undefined, 'No lint errors found.')
assert.equal(
report.message,
'Do not use an `<a>` element to navigate to `/photo/1/`. Use `<Link />` from `next/link` instead. See: https://nextjs.org/docs/messages/no-html-link-for-pages'
)
})
})
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default () => {}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default () => {}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default () => {}
1 change: 1 addition & 0 deletions test/unit/eslint-plugin-next/with-app-dir/app/page.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default () => {}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default () => {}
Loading