fix(lint): no-html-link in appDir (#68770)

supersedes #51783 ## What This PR fixes the issue of `<a>` tags for internal routing not being caught by linting in the app directory. ## Why The linting of the html links should be consistent across pages and app directories. ## How - Added a new function to parse the URLs in the appDir (since it's a bit different from the /pages directory) - Lint now checks against pagesUrls and appUrls - Added tests for the appDir in the no-html-link-for-pages test file fixes #51742 --------- Co-authored-by: JJ Kasper <[email protected]>
vercel · Sep 17, 2024 · c07dbd7 · c07dbd7
1 parent e881962
commit c07dbd7
Show file tree

Hide file tree

Showing 9 changed files with 268 additions and 2 deletions.
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
@@ -7,6 +7,7 @@ import {
   getUrlFromPagesDirectories,
   normalizeURL,
   execOnce,
+  getUrlFromAppDirectory,
 } from '../utils/url'
 
 const pagesDirWarning = execOnce((pagesDirs) => {
@@ -93,6 +94,9 @@ export = defineRule({
     }
 
     const pageUrls = getUrlFromPagesDirectories('/', foundPagesDirs)
+    const appDirUrls = getUrlFromAppDirectory('/', foundAppDirs)
+    const allUrls = [...pageUrls, ...appDirUrls]
+
     return {
       JSXOpeningElement(node) {
         if (node.name.name !== 'a') {
@@ -134,8 +138,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}`,

diff --git a/packages/eslint-plugin-next/src/utils/url.ts b/packages/eslint-plugin-next/src/utils/url.ts
@@ -33,6 +33,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 `/`
@@ -54,6 +81,56 @@ export function normalizeURL(url: string) {
   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
+      }
+
+      // The last segment (if it's a leaf) should be ignored.
+      if (
+        (segment === 'page' || segment === 'route') &&
+        index === segments.length - 1
+      ) {
+        return pathname
+      }
+
+      return `${pathname}/${segment}`
+    }, '')
+  )
+}
+
 /**
  * Gets the possible URLs from a directory.
  */
@@ -77,6 +154,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 {
@@ -91,3 +189,11 @@ export function execOnce<TArgs extends any[], TResult>(
     return result
   }
 }
+
+function ensureLeadingSlash(route: string) {
+  return route.startsWith('/') ? route : `/${route}`
+}
+
+function isGroupSegment(segment: string) {
+  return segment[0] === '(' && segment.endsWith(')')
+}
diff --git a/test/unit/eslint-plugin-next/no-html-link-for-pages.test.ts b/test/unit/eslint-plugin-next/no-html-link-for-pages.test.ts
@@ -225,7 +225,33 @@ 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('does not print warning when there are "pages" or "app" directories with rootDir in context settings', function () {
     const consoleSpy = jest.spyOn(console, 'warn').mockImplementation()
@@ -398,4 +424,129 @@ 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'
+    )
+  })
 })
diff --git a/...nt-plugin-next/with-app-dir/app/index.jsx → ...pp-dir/app/@modal/(..)photo/[id]/page.tsx b/...nt-plugin-next/with-app-dir/app/index.jsx → ...pp-dir/app/@modal/(..)photo/[id]/page.tsx
diff --git a/test/unit/eslint-plugin-next/with-app-dir/app/@modal/(..)photo/page.tsx b/test/unit/eslint-plugin-next/with-app-dir/app/@modal/(..)photo/page.tsx
@@ -0,0 +1 @@
+export default () => {}
diff --git a/test/unit/eslint-plugin-next/with-app-dir/app/[profile]/page.tsx b/test/unit/eslint-plugin-next/with-app-dir/app/[profile]/page.tsx
@@ -0,0 +1 @@
+export default () => {}
diff --git a/test/unit/eslint-plugin-next/with-app-dir/app/list/[foo]/[id].tsx b/test/unit/eslint-plugin-next/with-app-dir/app/list/[foo]/[id].tsx
@@ -0,0 +1 @@
+export default () => {}
diff --git a/test/unit/eslint-plugin-next/with-app-dir/app/page.jsx b/test/unit/eslint-plugin-next/with-app-dir/app/page.jsx
@@ -0,0 +1 @@
+export default () => {}
diff --git a/test/unit/eslint-plugin-next/with-app-dir/app/photo/[id]/page.tsx b/test/unit/eslint-plugin-next/with-app-dir/app/photo/[id]/page.tsx
@@ -0,0 +1 @@
+export default () => {}