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

feat(linter): eslint-plugin-next/no-duplicate-head #2438

Closed
wants to merge 4 commits into from
Closed

feat(linter): eslint-plugin-next/no-duplicate-head #2438

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
189d551
feat: eslint-plugin-next/no-duplicate-head
IWANABETHATGUY Feb 19, 2024
9523a61
fix: 🐛 typos
IWANABETHATGUY Feb 19, 2024
42a4e32
chore: 🤖 add some docs
IWANABETHATGUY Feb 20, 2024
02e2f13
fix: 🐛 update error message
IWANABETHATGUY Feb 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions crates/oxc_ast/src/ast/js.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,14 @@ impl<'a> Expression<'a> {
_ => false,
}
}

pub fn as_identifier(&self) -> Option<&Box<'a, IdentifierReference>> {
if let Self::Identifier(v) = self {
Some(v)
} else {
None
}
}
}

/// Identifier Name
Expand Down
20 changes: 20 additions & 0 deletions crates/oxc_ast/src/ast/jsx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,16 @@ pub enum JSXElementName<'a> {
MemberExpression(Box<'a, JSXMemberExpression<'a>>),
}

impl<'a> JSXElementName<'a> {
pub fn as_identifier(&self) -> Option<&JSXIdentifier> {
if let Self::Identifier(v) = self {
Some(v)
} else {
None
}
}
}

/// JSX Namespaced Name
#[derive(Debug, Hash)]
#[cfg_attr(feature = "serde", derive(Serialize), serde(tag = "type"))]
Expand Down Expand Up @@ -237,6 +247,16 @@ pub enum JSXChild<'a> {
Spread(JSXSpreadChild<'a>),
}

impl<'a> JSXChild<'a> {
pub fn as_element(&self) -> Option<&Box<'a, JSXElement<'a>>> {
if let Self::Element(v) = self {
Some(v)
} else {
None
}
}
}

#[derive(Debug, Hash)]
#[cfg_attr(feature = "serde", derive(Serialize), serde(tag = "type"))]
#[cfg_attr(all(feature = "serde", feature = "wasm"), derive(tsify::Tsify))]
Expand Down
2 changes: 2 additions & 0 deletions crates/oxc_linter/src/rules.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,7 @@ mod nextjs {
pub mod no_before_interactive_script_outside_document;
pub mod no_css_tags;
pub mod no_document_import_in_page;
pub mod no_duplicate_head;
pub mod no_head_element;
pub mod no_head_import_in_document;
pub mod no_img_element;
Expand Down Expand Up @@ -596,4 +597,5 @@ oxc_macros::declare_all_lint_rules! {
nextjs::no_document_import_in_page,
nextjs::no_unwanted_polyfillio,
nextjs::no_before_interactive_script_outside_document,
nextjs::no_duplicate_head,
}
215 changes: 215 additions & 0 deletions crates/oxc_linter/src/rules/nextjs/no_duplicate_head.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,215 @@
use oxc_ast::{
ast::{Expression, ImportDeclarationSpecifier::ImportDefaultSpecifier},
AstKind,
};
use oxc_diagnostics::{
miette::{self, Diagnostic},
thiserror::Error,
};
use oxc_macros::declare_oxc_lint;
use oxc_span::{GetSpan, Span};

use crate::{context::LintContext, rule::Rule};

#[derive(Debug, Error, Diagnostic)]
#[error("eslint-plugin-next(no-duplicate-head):")]
Copy link
Member

Choose a reason for hiding this comment

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

Need a error message here.

#[diagnostic(severity(warning), help("Do not include multiple instances of `<Head/>`. See: https://nextjs.org/docs/messages/no-duplicate-head"))]
struct NoDuplicateHeadDiagnostic(#[label] pub Span);

#[derive(Debug, Default, Clone)]
pub struct NoDuplicateHead;

declare_oxc_lint!(
/// ### What it does
/// Prevent duplicate usage of <Head> in pages/_document.js.
///
/// ### Why is this bad?
/// This can cause unexpected behavior in your application.
///
/// ### Example
/// ```javascript
/// import Document, { Html, Head, Main, NextScript } from 'next/document'
/// class MyDocument extends Document {
/// static async getInitialProps(ctx) {
/// }
/// render() {
/// return (
/// <Html>
/// <Head />
/// <body>
/// <Main />
/// <NextScript />
/// </body>
/// </Html>
/// )
/// }
///}
///export default MyDocument
/// ```
NoDuplicateHead,
correctness
);

impl Rule for NoDuplicateHead {
fn run_once(&self, ctx: &LintContext) {
let mut document_import_name = None;
let nodes = ctx.semantic().nodes();
for node in nodes.iter() {
match node.kind() {
oxc_ast::AstKind::ImportDeclaration(decl) => {
Copy link
Member

@Dunqing Dunqing Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Here we can use AstKind:ImportDefaultSpecifier instead, the parent kind of the AstKind:ImportDefaultSpecifier is AstKind::ImportDeclaration

if decl.source.value == "next/document" {
let document_import = decl.specifiers.as_ref().and_then(|specifiers| {
specifiers.iter().find(|spec| matches!(spec, ImportDefaultSpecifier(_)))
});
if let Some(ImportDefaultSpecifier(document_import_specifier)) =
document_import
{
document_import_name =
Some(document_import_specifier.local.name.clone());
}
}
}
oxc_ast::AstKind::ReturnStatement(stmt) => {
let document_class_id = nodes.ancestors(node.id()).find(|node_id| {
matches!(nodes.get_node(*node_id).kind(),
AstKind::Class(class)
if class
.super_class
.as_ref()
.and_then(|sc| sc.as_identifier())
.map(|id| &id.name)
== document_import_name.as_ref())
});
Comment on lines +77 to +87
Copy link
Member

@Dunqing Dunqing Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think we can have a simpler way to find the class, for example via ctx.semantic.classes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this would be faster, you still need to figure out if the class is the ancestor of current ReturnStatement

if document_class_id.map(|id| nodes.get_node(id)).is_none() {
continue;
}

let Some(Expression::JSXElement(ref element)) =
stmt.argument.as_ref().map(oxc_ast::ast::Expression::get_inner_expression)
else {
continue;
};
let head_components = element
.children
.iter()
.filter(|child| {
child
.as_element()
.and_then(|e| e.opening_element.name.as_identifier())
.map(|id| id.name == "Head")
.unwrap_or_default()
})
.collect::<Vec<_>>();
if head_components.len() > 1 {
for component in head_components {
ctx.diagnostic(NoDuplicateHeadDiagnostic(component.span()));
}
}
}
_ => continue,
}
}
}
}

#[test]
fn test() {
use crate::tester::Tester;

let pass = vec![
r"import Document, { Html, Head, Main, NextScript } from 'next/document'

class MyDocument extends Document {
static async getInitialProps(ctx) {
//...
}

render() {
return (
<Html>
<Head/>
</Html>
)
}
}

export default MyDocument
",
r#"import Document, { Html, Head, Main, NextScript } from 'next/document'

class MyDocument extends Document {
render() {
return (
<Html>
<Head>
<meta charSet="utf-8" />
<link
href="https://fonts.googleapis.com/css2?family=Sarabun:ital,wght@0,400;0,700;1,400;1,700&display=swap"
rel="stylesheet"
/>
</Head>
</Html>
)
}
}

export default MyDocument
"#,
];

let fail = vec![
r"
import Document, { Html, Main, NextScript } from 'next/document'
import Head from 'next/head'

class MyDocument extends Document {
render() {
return (
<Html>
<Head />
<Head />
<Head />
</Html>
)
}
}

export default MyDocument
",
r"
import Document, { Html, Main, NextScript } from 'next/document'
import Head from 'next/head'

class MyDocument extends Document {
render() {
return (
<Html>
<Head>
<meta charSet='utf-8' />
<link
href='https://fonts.googleapis.com/css2?family=Sarabun:ital,wght@0,400;0,700;1,400;1,700&display=swap'
rel='stylesheet'
/>
</Head>
<body>
<Main />
<NextScript />
</body>
<Head>
<script
dangerouslySetInnerHTML={{
__html: '',
}}
/>
</Head>
</Html>
)
}
}

export default MyDocument
",
];

Tester::new(NoDuplicateHead::NAME, pass, fail).test_and_snapshot();
}
60 changes: 60 additions & 0 deletions crates/oxc_linter/src/snapshots/no_duplicate_head.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
---
source: crates/oxc_linter/src/tester.rs
expression: no_duplicate_head
---

⚠ eslint-plugin-next(no-duplicate-head):
╭─[no_duplicate_head.tsx:9:27]
8 │ <Html>
9 │ <Head />
· ────────
10 │ <Head />
╰────
help: Do not include multiple instances of `<Head/>`. See: https://nextjs.org/docs/messages/no-duplicate-head

⚠ eslint-plugin-next(no-duplicate-head):
╭─[no_duplicate_head.tsx:10:27]
9 │ <Head />
10 │ <Head />
· ────────
11 │ <Head />
╰────
help: Do not include multiple instances of `<Head/>`. See: https://nextjs.org/docs/messages/no-duplicate-head

⚠ eslint-plugin-next(no-duplicate-head):
╭─[no_duplicate_head.tsx:11:27]
10 │ <Head />
11 │ <Head />
· ────────
12 │ </Html>
╰────
help: Do not include multiple instances of `<Head/>`. See: https://nextjs.org/docs/messages/no-duplicate-head

⚠ eslint-plugin-next(no-duplicate-head):
╭─[no_duplicate_head.tsx:9:27]
8 │ <Html>
9 │ ╭─▶ <Head>
10 │ │ <meta charSet='utf-8' />
11 │ │ <link
12 │ │ href='https://fonts.googleapis.com/css2?family=Sarabun:ital,wght@0,400;0,700;1,400;1,700&display=swap'
13 │ │ rel='stylesheet'
14 │ │ />
15 │ ╰─▶ </Head>
16 │ <body>
╰────
help: Do not include multiple instances of `<Head/>`. See: https://nextjs.org/docs/messages/no-duplicate-head

⚠ eslint-plugin-next(no-duplicate-head):
╭─[no_duplicate_head.tsx:20:27]
19 │ </body>
20 │ ╭─▶ <Head>
21 │ │ <script
22 │ │ dangerouslySetInnerHTML={{
23 │ │ __html: '',
24 │ │ }}
25 │ │ />
26 │ ╰─▶ </Head>
27 │ </Html>
╰────
help: Do not include multiple instances of `<Head/>`. See: https://nextjs.org/docs/messages/no-duplicate-head

Loading