[ESLint Plugin] Updates no-document-import-in-page rule to use path separators
#28768
Conversation
housseindjirdeh
requested review from
huozhi,
ijjk,
padmaia,
shuding,
styfle and
timneutkens
as code owners
ijjk
added
created-by: Chrome Aurora
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
@housseindjirdeh - I think you confuse path.sep and path.posix.sep in the unit test. Can you fix?
path.sep === / and path.posix.sep === \\
sorry, i'm the one who got confused. Your changes are correct.
on windows,
path.sep === \\ (and path.posix.sep === /)
| @@ -40,7 +42,7 @@ ruleTester.run('no-document-import-in-page', rule, { | |||
| } | |||
| } | |||
| `, | |||
| filename: 'pages\\_document.js', | |||
| filename: `pages${path.sep}_document.js`, | |||
There was a problem hiding this comment.
this should be path.posix.sep
path.sep === / and path.posix.sep === \\
edit
this is correct. path.sep === \\ and path.posix.sep === /
There was a problem hiding this comment.
path.sep is \\ on windows https://nodejs.org/api/path.html#path_path_sep
There was a problem hiding this comment.
thanks for catching my mistake. path.posix.sep is /. sorry about this @hussainanjar.
| page.startsWith('/_document') || | ||
| page.startsWith('\\_document') | ||
| page.startsWith(`${path.sep}_document`) || | ||
| page.startsWith(`${path.posix.sep}_document`) |
There was a problem hiding this comment.
Did you test this on Windows?
For what I can tell, this will behave identical to the existing code.
There was a problem hiding this comment.
Didn't test it on Windows (don't have a VM I can use unfortunately), but I didn't submit this change to fix the particular issue of it it still failing in some instances. Just thought it would make more sense to use path.sep as @kimbaudi suggested even though it'll likely behave the exact same way.
There was a problem hiding this comment.
sorry, i'm the one that was confused with path.sep and path.posix.sep. I just tested on Windows and path.sep is \\ and path.posix.sep is /. and yes, this will behave same as existing code.
There was a problem hiding this comment.
Just had a build on Windows fail in CI (GitHub actions) due to this exact rule:
Failed to compile.
./pages/_document.js
3:1 Error: next/document should not be imported outside of pages/_document.js. See https://nextjs.org/docs/messages/no-document-import-in-page. @next/next/no-document-import-in-page
Edit: ./pages/_document.js was the page it was attempting to build when the failure occurred.
There was a problem hiding this comment.
Just looked across the code, actually it seems to check out. So, now I'm not entirely sure what's gone wrong...
Stats from current PRDefault Build (Decrease detected ✓)General
Page Load Tests Overall decrease
|
| vercel/next.js canary | housseindjirdeh/next.js bugfix/#28596 | Change | |
|---|---|---|---|
| / failed reqs | 0 | 0 | ✓ |
| / total time (seconds) | 2.372 | 2.348 | -0.02 |
| / avg req/sec | 1053.94 | 1064.62 | +10.68 |
| /error-in-render failed reqs | 0 | 0 | ✓ |
| /error-in-render total time (seconds) | 1.291 | 1.31 | |
| /error-in-render avg req/sec | 1937.13 | 1908.09 |
Client Bundles (main, webpack, commons)
| vercel/next.js canary | housseindjirdeh/next.js bugfix/#28596 | 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 | housseindjirdeh/next.js bugfix/#28596 | Change | |
|---|---|---|---|
| polyfills-a4..dd70.js gzip | 31 kB | 31 kB | ✓ |
| Overall change | 31 kB | 31 kB | ✓ |
Client Pages
| vercel/next.js canary | housseindjirdeh/next.js bugfix/#28596 | 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 | housseindjirdeh/next.js bugfix/#28596 | Change | |
|---|---|---|---|
| _buildManifest.js gzip | 492 B | 492 B | ✓ |
| Overall change | 492 B | 492 B | ✓ |
Rendered Page Sizes
| vercel/next.js canary | housseindjirdeh/next.js bugfix/#28596 | Change | |
|---|---|---|---|
| index.html gzip | 541 B | 541 B | ✓ |
| link.html gzip | 553 B | 553 B | ✓ |
| withRouter.html gzip | 533 B | 533 B | ✓ |
| Overall change | 1.63 kB | 1.63 kB | ✓ |
Webpack 4 Mode (Increase detected ⚠️ )
General
| vercel/next.js canary | housseindjirdeh/next.js bugfix/#28596 | Change | |
|---|---|---|---|
| buildDuration | 9.7s | 9.8s | |
| buildDurationCached | 4s | 4.1s | |
| nodeModulesSize | 49.1 MB | 49.1 MB | ✓ |
Page Load Tests Overall increase ✓
| vercel/next.js canary | housseindjirdeh/next.js bugfix/#28596 | Change | |
|---|---|---|---|
| / failed reqs | 0 | 0 | ✓ |
| / total time (seconds) | 2.338 | 2.367 | |
| / avg req/sec | 1069.13 | 1056.16 | |
| /error-in-render failed reqs | 0 | 0 | ✓ |
| /error-in-render total time (seconds) | 1.312 | 1.298 | -0.01 |
| /error-in-render avg req/sec | 1906.14 | 1925.51 | +19.37 |
Client Bundles (main, webpack, commons)
| vercel/next.js canary | housseindjirdeh/next.js bugfix/#28596 | 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 | housseindjirdeh/next.js bugfix/#28596 | Change | |
|---|---|---|---|
| polyfills-a4..dd70.js gzip | 31 kB | 31 kB | ✓ |
| Overall change | 31 kB | 31 kB | ✓ |
Client Pages
| vercel/next.js canary | housseindjirdeh/next.js bugfix/#28596 | 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 | housseindjirdeh/next.js bugfix/#28596 | Change | |
|---|---|---|---|
| _buildManifest.js gzip | 500 B | 500 B | ✓ |
| Overall change | 500 B | 500 B | ✓ |
Rendered Page Sizes
| vercel/next.js canary | housseindjirdeh/next.js bugfix/#28596 | Change | |
|---|---|---|---|
| index.html gzip | 584 B | 584 B | ✓ |
| link.html gzip | 596 B | 596 B | ✓ |
| withRouter.html gzip | 577 B | 577 B | ✓ |
| Overall change | 1.76 kB | 1.76 kB | ✓ |
* 'canary' of github.com:filoblu/next.js: Reuse warning from postcss-loader (vercel#28727) Add RenderResult class (vercel#28776) v11.1.3-canary.8 feat: Adding generic typing for previewData (vercel#28668) Change create-next-app getting started text in TypeScript template (vercel#28718) [ESLint Plugin] Updates `no-document-import-in-page` rule to use `path` separators (vercel#28768) Update to new npm publish token (vercel#28772) Ensure build trace handles mixed modules (vercel#28770)
Context: https://github.com/vercel/next.js/pull/28745/files#r701418932