Skip to content

Commit 1c15288

Browse files
Copilotcamc314
andauthored
refactor(linter): extract duplicated is_jsx_fragment function to shared utils (#13093)
This PR addresses review comments from PR #12988 by extracting the duplicated `is_jsx_fragment` function that was present in both `jsx_fragments.rs` and `jsx_no_useless_fragment.rs` rules. ## Changes Made ### Code Deduplication - **Extracted** `is_jsx_fragment` function from both React linting rules - **Moved** the function to `crates/oxc_linter/src/utils/react.rs` as a shared utility - **Updated** both `jsx_fragments` and `jsx_no_useless_fragment` rules to use the shared function - **Removed** duplicate implementations and cleaned up unused imports ### Self-Closing Fragment Handling The review also mentioned handling self-closing fragments (e.g., `<Fragment />`). Upon investigation, this was already correctly implemented in the original code: ```rust let Some(closing_element) = &jsx_elem.closing_element else { return; // Skip self-closing elements }; ``` Self-closing fragments cannot be converted to shorthand syntax (`<></>`) and should not trigger the rule, which is the correct behavior already in place. ## Function Details The extracted `is_jsx_fragment` function recognizes both forms of React fragments: - `<Fragment>` (identifier reference) - `<React.Fragment>` (member expression) ```rust pub fn is_jsx_fragment(elem: &JSXOpeningElement) -> bool { match &elem.name { JSXElementName::IdentifierReference(ident) => ident.name == "Fragment", JSXElementName::MemberExpression(mem_expr) => { if let JSXMemberExpressionObject::IdentifierReference(ident) = &mem_expr.object { ident.name == "React" && mem_expr.property.name == "Fragment" } else { false } } _ => false, } } ``` ## Testing - ✅ All existing tests continue to pass (755/755) - ✅ Specific jsx-related tests pass (49/49) - ✅ Both `jsx_fragments` and `jsx_no_useless_fragment` rules maintain their behavior - ✅ Self-closing fragment test cases verify correct behavior (included in existing test suite) This refactoring eliminates code duplication while maintaining full backward compatibility and correctness of both linting rules. <!-- START COPILOT CODING AGENT TIPS --> --- 💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click [here](https://survey.alchemer.com/s3/8343779/Copilot-Coding-agent) to start the survey. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: camc314 <18101008+camc314@users.noreply.github.com>
1 parent 0b61338 commit 1c15288

File tree

3 files changed

+22
-38
lines changed

3 files changed

+22
-38
lines changed

crates/oxc_linter/src/rules/react/jsx_fragments.rs

Lines changed: 2 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,10 @@
1-
use oxc_ast::{
2-
AstKind,
3-
ast::{JSXElementName, JSXMemberExpressionObject, JSXOpeningElement},
4-
};
1+
use oxc_ast::AstKind;
52
use oxc_diagnostics::OxcDiagnostic;
63
use oxc_macros::declare_oxc_lint;
74
use oxc_span::{GetSpan, Span};
85
use serde_json::Value;
96

10-
use crate::{AstNode, context::LintContext, rule::Rule};
7+
use crate::{AstNode, context::LintContext, rule::Rule, utils::is_jsx_fragment};
118

129
fn jsx_fragments_diagnostic(span: Span, mode: FragmentMode) -> OxcDiagnostic {
1310
let msg = if mode == FragmentMode::Element {
@@ -178,22 +175,6 @@ impl Rule for JsxFragments {
178175
}
179176
}
180177

181-
fn is_jsx_fragment(elem: &JSXOpeningElement) -> bool {
182-
match &elem.name {
183-
JSXElementName::IdentifierReference(ident) => ident.name == "Fragment",
184-
JSXElementName::MemberExpression(mem_expr) => {
185-
if let JSXMemberExpressionObject::IdentifierReference(ident) = &mem_expr.object {
186-
ident.name == "React" && mem_expr.property.name == "Fragment"
187-
} else {
188-
false
189-
}
190-
}
191-
JSXElementName::NamespacedName(_)
192-
| JSXElementName::Identifier(_)
193-
| JSXElementName::ThisExpression(_) => false,
194-
}
195-
}
196-
197178
#[test]
198179
fn test() {
199180
use crate::tester::Tester;

crates/oxc_linter/src/rules/react/jsx_no_useless_fragment.rs

Lines changed: 2 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,14 @@ use crate::{
33
context::{ContextHost, LintContext},
44
fixer::{RuleFix, RuleFixer},
55
rule::Rule,
6+
utils::is_jsx_fragment,
67
};
78
use oxc_allocator::Vec as ArenaVec;
89
use oxc_ast::{
910
AstKind,
1011
ast::{
1112
JSXAttributeItem, JSXAttributeName, JSXChild, JSXElement, JSXElementName, JSXExpression,
12-
JSXFragment, JSXMemberExpressionObject, JSXOpeningElement,
13+
JSXFragment,
1314
},
1415
};
1516
use oxc_diagnostics::OxcDiagnostic;
@@ -321,22 +322,6 @@ fn is_html_element(elem_name: &JSXElementName) -> bool {
321322
ident.name.starts_with(char::is_lowercase)
322323
}
323324

324-
fn is_jsx_fragment(elem: &JSXOpeningElement) -> bool {
325-
match &elem.name {
326-
JSXElementName::IdentifierReference(ident) => ident.name == "Fragment",
327-
JSXElementName::MemberExpression(mem_expr) => {
328-
if let JSXMemberExpressionObject::IdentifierReference(ident) = &mem_expr.object {
329-
ident.name == "React" && mem_expr.property.name == "Fragment"
330-
} else {
331-
false
332-
}
333-
}
334-
JSXElementName::NamespacedName(_)
335-
| JSXElementName::Identifier(_)
336-
| JSXElementName::ThisExpression(_) => false,
337-
}
338-
}
339-
340325
fn has_less_than_two_children(children: &oxc_allocator::Vec<'_, JSXChild<'_>>) -> bool {
341326
let non_padding_children = children.iter().filter(|v| is_padding_spaces(v)).collect::<Vec<_>>();
342327

crates/oxc_linter/src/utils/react.rs

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -333,3 +333,21 @@ pub fn is_react_function_call(call: &CallExpression, expected_call: &str) -> boo
333333
true
334334
}
335335
}
336+
337+
/// Checks if a JSX opening element is a React Fragment.
338+
/// Recognizes both `<Fragment>` and `<React.Fragment>` forms.
339+
pub fn is_jsx_fragment(elem: &JSXOpeningElement) -> bool {
340+
match &elem.name {
341+
JSXElementName::IdentifierReference(ident) => ident.name == "Fragment",
342+
JSXElementName::MemberExpression(mem_expr) => {
343+
if let JSXMemberExpressionObject::IdentifierReference(ident) = &mem_expr.object {
344+
ident.name == "React" && mem_expr.property.name == "Fragment"
345+
} else {
346+
false
347+
}
348+
}
349+
JSXElementName::NamespacedName(_)
350+
| JSXElementName::Identifier(_)
351+
| JSXElementName::ThisExpression(_) => false,
352+
}
353+
}

0 commit comments

Comments
 (0)