[ESLint Plugin] Updates `no-document-import-in-page` rule to use `path` separators #28768

housseindjirdeh · 2021-09-03T14:44:03Z

Context: https://github.com/vercel/next.js/pull/28745/files#r701418932

kimbaudi

~~@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 === /)

kimbaudi · 2021-09-03T14:57:47Z

test/unit/eslint-plugin-next/no-document-import-in-page.test.ts

@@ -40,7 +42,7 @@ ruleTester.run('no-document-import-in-page', rule, {
      }
    }
    `,
-      filename: 'pages\\_document.js',
+      filename: `pages${path.sep}_document.js`,


~~this should be path.posix.sep~~

~~path.sep === / and path.posix.sep === \\~~

edit

this is correct. path.sep === \\ and path.posix.sep === /

path.sep is \\ on windows https://nodejs.org/api/path.html#path_path_sep

thanks for catching my mistake. path.posix.sep is /. sorry about this @hussainanjar.

styfle · 2021-09-03T15:03:36Z

packages/eslint-plugin-next/lib/rules/no-document-import-in-page.js

-          page.startsWith('/_document') ||
-          page.startsWith('\\_document')
+          page.startsWith(`${path.sep}_document`) ||
+          page.startsWith(`${path.posix.sep}_document`)


Did you test this on Windows?

For what I can tell, this will behave identical to the existing code.

https://nodejs.org/api/path.html#path_path_sep

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.

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.

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.

Just looked across the code, actually it seems to check out. So, now I'm not entirely sure what's gone wrong...

ijjk · 2021-09-03T18:22:11Z

Stats from current PR

Default Build (Decrease detected ✓)

General

	vercel/next.js canary	housseindjirdeh/next.js bugfix/#28596	Change
buildDuration	12.1s	11.9s	-163ms
buildDurationCached	3.1s	3.1s	-48ms
nodeModulesSize	49.1 MB	49.1 MB	✓

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	⚠️ +0.02
/error-in-render avg req/sec	1937.13	1908.09	⚠️ -29.04

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	⚠️ +131ms
buildDurationCached	4s	4.1s	⚠️ +26ms
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	⚠️ +0.03
/ avg req/sec	1069.13	1056.16	⚠️ -12.97
/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	✓

Commit: a41bde4

* '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)

uses path.sep instead of literal slashes

5a1587d

housseindjirdeh requested review from huozhi, ijjk, padmaia, shuding, styfle and timneutkens as code owners September 3, 2021 14:44

ijjk added created-by: Chrome Aurora type: next labels Sep 3, 2021

housseindjirdeh mentioned this pull request Sep 3, 2021

Fix next lint no-document-import-in-page on Windows #28745

Merged

This comment has been minimized.

Sign in to view

huozhi approved these changes Sep 3, 2021

View reviewed changes

kimbaudi suggested changes Sep 3, 2021

View reviewed changes

styfle reviewed Sep 3, 2021

View reviewed changes

kimbaudi approved these changes Sep 3, 2021

View reviewed changes

Merge branch 'canary' into bugfix/#28596

a41bde4

kodiakhq bot merged commit d056312 into vercel:canary Sep 3, 2021

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

[ESLint Plugin] Updates `no-document-import-in-page` rule to use `path` separators #28768

[ESLint Plugin] Updates `no-document-import-in-page` rule to use `path` separators #28768

housseindjirdeh commented Sep 3, 2021 •

edited

Loading

This comment has been minimized.

kimbaudi left a comment •

edited

Loading

kimbaudi Sep 3, 2021 •

edited

Loading

styfle Sep 3, 2021 •

edited

Loading

kimbaudi Sep 3, 2021

styfle Sep 3, 2021

housseindjirdeh Sep 3, 2021

kimbaudi Sep 3, 2021

dburles Sep 29, 2021 •

edited

Loading

dburles Sep 29, 2021

ijjk commented Sep 3, 2021

[ESLint Plugin] Updates no-document-import-in-page rule to use path separators #28768

[ESLint Plugin] Updates no-document-import-in-page rule to use path separators #28768

Conversation

housseindjirdeh commented Sep 3, 2021 • edited Loading

This comment has been minimized.

kimbaudi left a comment • edited Loading

Choose a reason for hiding this comment

kimbaudi Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

styfle Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

kimbaudi Sep 3, 2021

Choose a reason for hiding this comment

styfle Sep 3, 2021

Choose a reason for hiding this comment

housseindjirdeh Sep 3, 2021

Choose a reason for hiding this comment

kimbaudi Sep 3, 2021

Choose a reason for hiding this comment

dburles Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

dburles Sep 29, 2021

Choose a reason for hiding this comment

ijjk commented Sep 3, 2021

Stats from current PR

[ESLint Plugin] Updates `no-document-import-in-page` rule to use `path` separators #28768

[ESLint Plugin] Updates `no-document-import-in-page` rule to use `path` separators #28768

housseindjirdeh commented Sep 3, 2021 •

edited

Loading

kimbaudi left a comment •

edited

Loading

kimbaudi Sep 3, 2021 •

edited

Loading

styfle Sep 3, 2021 •

edited

Loading

dburles Sep 29, 2021 •

edited

Loading