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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Sep 3, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a1587d
uses path.sep instead of literal slashes
housseindjirdeh Sep 3, 2021
a41bde4
Merge branch 'canary' into bugfix/#28596
samcx Sep 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions packages/eslint-plugin-next/lib/rules/no-document-import-in-page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
const path = require('path')

module.exports = {
meta: {
docs: {
Expand All @@ -18,8 +20,8 @@ module.exports = {

if (
!page ||
page.startsWith('/_document') ||
page.startsWith('\\_document')
page.startsWith(`${path.sep}_document`) ||
page.startsWith(`${path.posix.sep}_document`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@dburles dburles Sep 29, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

) {
return
}
Expand Down
8 changes: 5 additions & 3 deletions test/unit/eslint-plugin-next/no-document-import-in-page.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import path from 'path'

import rule from '@next/eslint-plugin-next/lib/rules/no-document-import-in-page'
import { RuleTester } from 'eslint'
;(RuleTester as any).setDefaultConfig({
Expand Down Expand Up @@ -40,7 +42,7 @@ ruleTester.run('no-document-import-in-page', rule, {
}
}
`,
filename: 'pages\\_document.js',
filename: `pages${path.sep}_document.js`,
Copy link

@kimbaudi kimbaudi Sep 3, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be path.posix.sep

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

edit

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

Copy link
Member

@styfle styfle Sep 3, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
{
code: `import NextDocument from "next/document"
Expand All @@ -54,7 +56,7 @@ ruleTester.run('no-document-import-in-page', rule, {
}
}
`,
filename: 'pages/_document.tsx',
filename: `pages${path.posix.sep}_document.tsx`,
},
{
code: `import Document from "next/document"
Expand Down Expand Up @@ -147,7 +149,7 @@ ruleTester.run('no-document-import-in-page', rule, {

export const Test = () => <p>Test</p>
`,
filename: 'pages\\test.js',
filename: `pages${path.sep}test.js`,
errors: [
{
message:
Expand Down