Bug fix for lint rule #28677

prateekbh · 2021-08-31T19:55:39Z

The no-html-link-for-pages.js rules fails for directory pattern /foo/[bar]/....
This PR fixes it and adds test for it.

Bug

Integration tests added

ijjk · 2021-08-31T20:16:51Z

packages/eslint-plugin-next/lib/utils/url.js

+  ).map((urlReg) => {
+    // /foo/[bar] filees fails without this.
+    urlReg = urlReg.replace(/\[/g, "\\[");
+    new RegExp(urlReg)


Suggested change

new RegExp(urlReg)

return new RegExp(urlReg)

Should we escape all regex characters like we do here?

next.js/packages/next/shared/lib/router/utils/route-regex.ts

Lines 9 to 11 in acf65f9

function escapeRegex(str: string) {

return str.replace(/[|\\{}()[\]^$+*?.-]/g, '\\$&')

}

I moved the logic around to handle this, we don't need to handle the escaping of all the items

added more tests to check all permutations and combinations

ijjk · 2021-09-01T10:14:36Z

Failing test suites

Commit: 09b3f7b

test/unit/eslint-plugin-next/no-html-link-for-pages.test.ts

no-html-link-for-pages > invalid dynamic route

Expand output

● no-html-link-for-pages › invalid dynamic route

assert.notEqual(received, expected)

Expected value to not be equal to:
  undefined
Received:
  undefined

Message:
  No lint errors found.

Difference:

Compared values have no visual difference.

  154 |       report.message,
  155 |       "Do not use the HTML <a> tag to navigate to /. Use Link from 'next/link' instead. See: https://nextjs.org/docs/messages/no-html-link-for-pages."
> 156 |     )
      |      ^
  157 |   })
  158 |
  159 |   it('invalid dynamic route', function () {

  at Object.<anonymous> (unit/eslint-plugin-next/no-html-link-for-pages.test.ts:156:25)

test/integration/app-document-remove-hmr/test/index.test.js

_app removal HMR > should HMR when _document is removed

Expand output

● _app removal HMR › should HMR when _document is removed

TIMED OUT: success

<head><meta charset="utf-8"><meta name="viewport" content="width=device-width"><meta name="next-head-count" content="2"><noscript data-n-css=""></noscript><script defer="" nomodule="" src="/_next/static/chunks/polyfills.js?ts=1630491448420"></script><script src="/_next/static/chunks/webpack.js?ts=1630491448420" defer=""></script><script src="/_next/static/chunks/main.js?ts=1630491448420" defer=""></script><script src="/_next/static/chunks/pages/_app.js?ts=1630491448420" defer=""></script><script src="/_next/static/chunks/pages/index.js?ts=1630491448420" defer=""></script><script src="/_next/static/development/_buildManifest.js?ts=1630491448420" defer=""></script><script src="/_next/static/development/_ssgManifest.js?ts=1630491448420" defer=""></script><noscript id="__next_css__DO_NOT_USE__"></noscript></head><body><p>custom _document</p><div id="__next"><p>custom _app</p><p>index page</p></div><script src="/_next/static/chunks/react-refresh.js?ts=1630491448420"></script><script id="__NEXT_DATA__" type="application/json">{"props":{"pageProps":{}},"page":"/","query":{},"buildId":"development","nextExport":true,"autoExport":true,"isFallback":false,"scriptLoader":[]}</script><div id="__next-build-watcher" style="position: fixed; bottom: 10px; right: 20px; width: 0px; height: 0px; z-index: 99999;"></div><next-route-announcer><p aria-live="assertive" id="__next-route-announcer__" role="alert" style="border: 0px; clip: rect(0px, 0px, 0px, 0px); height: 1px; margin: -1px; overflow: hidden; padding: 0px; position: absolute; width: 1px; white-space: nowrap; overflow-wrap: normal;"></p></next-route-announcer></body>

  438 |   write(content) {
  439 |     if (!this.originalContent) {
> 440 |       this.originalContent = content
      |               ^
  441 |     }
  442 |     writeFileSync(this.path, content, 'utf8')
  443 |   }

  at Object.check (lib/next-test-utils.js:440:15)
  at Object.<anonymous> (integration/app-document-remove-hmr/test/index.test.js:78:13)

housseindjirdeh · 2021-09-01T16:24:04Z

packages/eslint-plugin-next/lib/utils/url.js

@@ -73,6 +75,9 @@ function parseUrlForPages(urlprefix, directory) {
    }
  })
  return res
+  // return res.map((expression) =>
+  //   expression.replace(/[|\\{}()[\]^$+*?.-]/g, '\\$&')
+  // )


Should this commented code be here?

nope just waiting for the tests to pass

housseindjirdeh

Thanks for putting up a fix for this @prateekbh :)

LGTM (aside from the commented out code) ✅

ijjk · 2021-09-01T19:41:57Z

Stats from current PR

Default Build (Decrease detected ✓)

General

	vercel/next.js canary	prateekbh/next.js lint-fix	Change
buildDuration	11.7s	11.3s	-372ms
buildDurationCached	2.9s	2.7s	-134ms
nodeModulesSize	50.4 MB	50.4 MB	✓

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	prateekbh/next.js lint-fix	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.248	2.295	⚠️ +0.05
/ avg req/sec	1111.93	1089.31	⚠️ -22.62
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.229	1.325	⚠️ +0.1
/error-in-render avg req/sec	2033.39	1887.18	⚠️ -146.21

Client Bundles (main, webpack, commons)

	vercel/next.js canary	prateekbh/next.js lint-fix	Change
745.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	23.3 kB	23.3 kB	✓
webpack-HASH.js gzip	1.45 kB	1.45 kB	✓
Overall change	67.2 kB	67.2 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	prateekbh/next.js lint-fix	Change
polyfills-a4..dd70.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	prateekbh/next.js lint-fix	Change
_app-HASH.js gzip	979 B	979 B	✓
_error-HASH.js gzip	194 B	194 B	✓
amp-HASH.js gzip	312 B	312 B	✓
css-HASH.js gzip	329 B	329 B	✓
dynamic-HASH.js gzip	2.67 kB	2.67 kB	✓
head-HASH.js gzip	351 B	351 B	✓
hooks-HASH.js gzip	918 B	918 B	✓
image-HASH.js gzip	4.14 kB	4.14 kB	✓
index-HASH.js gzip	261 B	261 B	✓
link-HASH.js gzip	1.66 kB	1.66 kB	✓
routerDirect..HASH.js gzip	318 B	318 B	✓
script-HASH.js gzip	387 B	387 B	✓
withRouter-HASH.js gzip	320 B	320 B	✓
bb14e60e810b..30f.css gzip	125 B	125 B	✓
Overall change	13 kB	13 kB	✓

Client Build Manifests

	vercel/next.js canary	prateekbh/next.js lint-fix	Change
_buildManifest.js gzip	491 B	491 B	✓
Overall change	491 B	491 B	✓

Rendered Page Sizes

	vercel/next.js canary	prateekbh/next.js lint-fix	Change
index.html gzip	540 B	540 B	✓
link.html gzip	552 B	552 B	✓
withRouter.html gzip	533 B	533 B	✓
Overall change	1.63 kB	1.63 kB	✓

Webpack 4 Mode (Decrease detected ✓)

General

	vercel/next.js canary	prateekbh/next.js lint-fix	Change
buildDuration	9.3s	9.1s	-198ms
buildDurationCached	4.3s	3.8s	-508ms
nodeModulesSize	50.4 MB	50.4 MB	✓

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	prateekbh/next.js lint-fix	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.286	2.285	0
/ avg req/sec	1093.73	1093.87	+0.14
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.286	1.374	⚠️ +0.09
/error-in-render avg req/sec	1943.83	1819.63	⚠️ -124.2

Client Bundles (main, webpack, commons)

	vercel/next.js canary	prateekbh/next.js lint-fix	Change
16.HASH.js gzip	186 B	186 B	✓
677f882d2ed8..HASH.js gzip	14.1 kB	14.1 kB	✓
framework.HASH.js gzip	41.9 kB	41.9 kB	✓
main-HASH.js gzip	10.7 kB	10.7 kB	✓
webpack-HASH.js gzip	1.19 kB	1.19 kB	✓
Overall change	68.1 kB	68.1 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	prateekbh/next.js lint-fix	Change
polyfills-a4..dd70.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	prateekbh/next.js lint-fix	Change
_app-HASH.js gzip	964 B	964 B	✓
_error-HASH.js gzip	3.8 kB	3.8 kB	✓
amp-HASH.js gzip	552 B	552 B	✓
css-HASH.js gzip	333 B	333 B	✓
dynamic-HASH.js gzip	2.87 kB	2.87 kB	✓
head-HASH.js gzip	3.06 kB	3.06 kB	✓
hooks-HASH.js gzip	924 B	924 B	✓
index-HASH.js gzip	231 B	231 B	✓
link-HASH.js gzip	1.64 kB	1.64 kB	✓
routerDirect..HASH.js gzip	298 B	298 B	✓
script-HASH.js gzip	3.03 kB	3.03 kB	✓
withRouter-HASH.js gzip	295 B	295 B	✓
30809af5c834..565.css gzip	125 B	125 B	✓
Overall change	18.1 kB	18.1 kB	✓

Client Build Manifests

	vercel/next.js canary	prateekbh/next.js lint-fix	Change
_buildManifest.js gzip	500 B	500 B	✓
Overall change	500 B	500 B	✓

Rendered Page Sizes

	vercel/next.js canary	prateekbh/next.js lint-fix	Change
index.html gzip	586 B	586 B	✓
link.html gzip	597 B	597 B	✓
withRouter.html gzip	578 B	578 B	✓
Overall change	1.76 kB	1.76 kB	✓

Commit: 0394a43

prateekbh added 2 commits September 1, 2021 01:20

linter bug fix

6f72cc4

fixing test

9d97df6

prateekbh requested review from huozhi, ijjk, padmaia, shuding, styfle and timneutkens as code owners August 31, 2021 19:55

fixing error message

751f71f

This comment has been minimized.

Sign in to view

ijjk reviewed Aug 31, 2021

View reviewed changes

fixing tests

78e095e

prateekbh marked this pull request as draft September 1, 2021 10:08

Merge branch 'canary' into lint-fix

09b3f7b

This comment has been minimized.

Sign in to view

fixing tests

15d7168

This comment has been minimized.

Sign in to view

housseindjirdeh reviewed Sep 1, 2021

View reviewed changes

housseindjirdeh previously approved these changes Sep 1, 2021

View reviewed changes

Update url.js

1176639

prateekbh dismissed housseindjirdeh’s stale review via 1176639 September 1, 2021 18:30

This comment has been minimized.

Sign in to view

prateekbh marked this pull request as ready for review September 1, 2021 19:31

Merge branch 'canary' into lint-fix

0394a43

timneutkens merged commit 7c29a26 into vercel:canary Sep 2, 2021

prateekbh deleted the lint-fix branch September 2, 2021 10:32

vercel locked as resolved and limited conversation to collaborators Jan 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug fix for lint rule #28677

Bug fix for lint rule #28677

prateekbh commented Aug 31, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

ijjk Aug 31, 2021 •

edited

Loading

prateekbh Sep 1, 2021

prateekbh Sep 1, 2021

ijjk commented Sep 1, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

housseindjirdeh Sep 1, 2021

prateekbh Sep 1, 2021

housseindjirdeh left a comment

This comment has been minimized.

ijjk commented Sep 1, 2021

	function escapeRegex(str: string) {
	return str.replace(/[\|\\{}()[\]^$+*?.-]/g, '\\$&')
	}

Bug fix for lint rule #28677

Bug fix for lint rule #28677

Conversation

prateekbh commented Aug 31, 2021 • edited Loading

Bug

This comment has been minimized.

This comment has been minimized.

ijjk Aug 31, 2021 • edited Loading

Choose a reason for hiding this comment

prateekbh Sep 1, 2021

Choose a reason for hiding this comment

prateekbh Sep 1, 2021

Choose a reason for hiding this comment

ijjk commented Sep 1, 2021 • edited Loading

Failing test suites

This comment has been minimized.

This comment has been minimized.

housseindjirdeh Sep 1, 2021

Choose a reason for hiding this comment

prateekbh Sep 1, 2021

Choose a reason for hiding this comment

housseindjirdeh left a comment

Choose a reason for hiding this comment

This comment has been minimized.

ijjk commented Sep 1, 2021

Stats from current PR

prateekbh commented Aug 31, 2021 •

edited

Loading

ijjk Aug 31, 2021 •

edited

Loading

ijjk commented Sep 1, 2021 •

edited

Loading