Skip to content

fix(linter): Fix the react/jsx-fragments rule config to take a string argument #16175

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+67 −45
Open

fix(linter): Fix the react/jsx-fragments rule config to take a string argument #16175

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7aab353
fix(linter): react/jsx-fragments rule should take string argument.
connorshea Nov 26, 2025
82c876f
Update the diagnostics to use backticks.
connorshea Nov 26, 2025
266e081
Allow a string arg or an object argument for backwards-compatibility.
connorshea Nov 26, 2025
84426a7
Clean up the from_configuration method a bit.
connorshea Nov 27, 2025
28d356a
Refactor to use an enum.
connorshea Nov 27, 2025
65f8344
Update to use DefaultRuleConfig for configuration parsing
connorshea Nov 27, 2025
5379f68
[autofix.ci] apply automated fixes
autofix-ci[bot] Nov 27, 2025
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
63 changes: 36 additions & 27 deletions crates/oxc_linter/src/rules/react/jsx_fragments.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,23 +10,26 @@ use crate::{AstNode, context::LintContext, rule::Rule, utils::is_jsx_fragment};

fn jsx_fragments_diagnostic(span: Span, mode: FragmentMode) -> OxcDiagnostic {
let msg = if mode == FragmentMode::Element {
"Standard form for React fragments is preferred"
"Standard form for React fragments is preferred."
} else {
"Shorthand form for React fragments is preferred"
"Shorthand form for React fragments is preferred."
};
let help = if mode == FragmentMode::Element {
"Use <React.Fragment></React.Fragment> instead of <></>"
"Use `<React.Fragment></React.Fragment>` instead of `<></>`."
} else {
"Use <></> instead of <React.Fragment></React.Fragment>"
"Use `<></>` instead of `<React.Fragment></React.Fragment>`."
};
OxcDiagnostic::warn(msg).with_help(help).with_label(span)
}

#[derive(Debug, Default, Clone, JsonSchema, Deserialize, Serialize)]
#[serde(rename_all = "camelCase", default)]
#[derive(Debug, Default, Clone)]
pub struct JsxFragments {
/// `syntax` mode:
///
mode: FragmentMode,
}
Copy link
Contributor

@PeterCardenas PeterCardenas Nov 27, 2025

Choose a reason for hiding this comment

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

you can support both by doing an enum, something like this i think

#[schemars(untagged)]
pub enum JsxFragments {
  StringFragmentMode(FragmentMode);
  ObjectFragmentMode({ mode: FragmentMode });
}

Copy link
Contributor Author

@connorshea connorshea Nov 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This almost works, but not quite, as it breaks the self.mode throughout the file. I can do it like this and it compiles, but unwrapping in from_configuration apparently panics in some cases.

#[derive(Debug, Clone, JsonSchema, Deserialize)]
#[serde(untagged)]
pub enum JsxFragments {
    Mode(FragmentMode),
    Object { mode: FragmentMode },
}

impl Default for JsxFragments {
    fn default() -> Self {
        JsxFragments::Mode(FragmentMode::Syntax)
    }
}

impl JsxFragments {
    fn mode(&self) -> FragmentMode {
        match self {
            JsxFragments::Mode(m) => *m,
            JsxFragments::Object { mode } => *mode,
        }
    }
}

Copy link
Contributor Author

@connorshea connorshea Nov 27, 2025

Choose a reason for hiding this comment

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

unwrap_or_default in from_configuration fixes the panic! Although I have apparently broken the behavior in the tests somehow. 🤔

Copy link
Contributor

@PeterCardenas PeterCardenas Nov 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

ah hm didn't consider that it makes the code weird, might need a helper function to convert the mode from the enum to the fragment mode
edit: oh i see that's what you did

Copy link
Contributor

@PeterCardenas PeterCardenas Nov 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

hm unwrap would be preferable i think to enforce that the config they pass in is correct. panicking is a bit harsh but that's a separate problem

Copy link
Contributor Author

@connorshea connorshea Nov 27, 2025

Choose a reason for hiding this comment

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

I've pushed up a commit to use this pattern, although I'm not really sure it's particularly better than the previous version. I'll probably let cam decide on whatever is preferred.

Copy link
Contributor

@PeterCardenas PeterCardenas Nov 27, 2025

Choose a reason for hiding this comment

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

imo it's better because it codifies the config in the type rather than some adhoc deserialization code

Copy link
Contributor

@PeterCardenas PeterCardenas Nov 27, 2025

Choose a reason for hiding this comment

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

also you can get config doc generation hopefully that describes these two options if you put this enum in the config value

Copy link
Contributor Author

@connorshea connorshea Nov 27, 2025

Choose a reason for hiding this comment

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

Unfortunately we'll need to make it possible to serialize a value of this type for the website to generate it, right now it can't handle an enum that is more than just simple types.

Copy link
Contributor Author

@connorshea connorshea Nov 27, 2025

Choose a reason for hiding this comment

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

DefaultRuleConfig definitely makes it better: 65f8344


#[derive(Debug, Default, Clone, PartialEq, Eq, Copy, JsonSchema, Deserialize, Serialize)]
#[serde(rename_all = "kebab-case")]
pub enum FragmentMode {
/// This is the default mode. It will enforce the shorthand syntax for React fragments, with one exception.
/// Keys or attributes are not supported by the shorthand syntax, so the rule will not warn on standard-form fragments that use those.
///
Expand All @@ -43,8 +46,8 @@ pub struct JsxFragments {
/// ```jsx
/// <React.Fragment key="key"><Foo /></React.Fragment>
/// ```
///
/// `element` mode:
#[default]
Syntax,
/// This mode enforces the standard form for React fragments.
///
/// Examples of **incorrect** code for this rule:
Expand All @@ -60,14 +63,6 @@ pub struct JsxFragments {
/// ```jsx
/// <React.Fragment key="key"><Foo /></React.Fragment>
/// ```
mode: FragmentMode,
}

#[derive(Debug, Default, Clone, PartialEq, Eq, Copy, JsonSchema, Deserialize, Serialize)]
#[serde(rename_all = "kebab-case")]
pub enum FragmentMode {
#[default]
Syntax,
Element,
}

Expand All @@ -89,19 +84,26 @@ declare_oxc_lint!(
react,
style,
fix,
config = JsxFragments,
config = FragmentMode,
);

impl Rule for JsxFragments {
// Generally we should prefer the string-only syntax for compatibility with the original ESLint rule,
// but we originally implemented the rule with only the object syntax, so we support both now.
fn from_configuration(value: Value) -> Self {
let obj = value.get(0);
Self {
mode: obj
.and_then(|v| v.get("mode"))
.and_then(Value::as_str)
.map(FragmentMode::from)
.unwrap_or_default(),
}
let arg = value.get(0);
let mode = arg
.and_then(|v| {
// allow either a string argument, eg: ["syntax"], or an object with `mode` field
if let Some(s) = v.as_str() {
Some(FragmentMode::from(s))
} else {
v.get("mode").and_then(Value::as_str).map(FragmentMode::from)
}
})
.unwrap_or_default();

Self { mode }
}

fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
Expand Down Expand Up @@ -186,24 +188,31 @@ fn test() {
(r#"<React.Fragment key="key"><Foo /></React.Fragment>"#, None),
("<Fragment />", None),
("<React.Fragment />", None),
// Configuration can be done via a string directly, or an object with the `mode` field.
("<><Foo /></>", Some(json!(["syntax"]))),
("<><Foo /></>", Some(json!([{"mode": "syntax"}]))),
("<React.Fragment><Foo /></React.Fragment>", Some(json!(["element"]))),
("<React.Fragment><Foo /></React.Fragment>", Some(json!([{"mode": "element"}]))),
];

let fail = vec![
("<Fragment><Foo /></Fragment>", None),
("<React.Fragment><Foo /></React.Fragment>", None),
("<><Foo /></>", Some(json!(["element"]))),
("<><Foo /></>", Some(json!([{"mode": "element"}]))),
];

let fix = vec![
("<Fragment><Foo /></Fragment>", "<><Foo /></>", None),
("<React.Fragment><Foo /></React.Fragment>", "<><Foo /></>", None),
("<><Foo /></>", "<React.Fragment><Foo /></React.Fragment>", Some(json!(["element"]))),
(
"<><Foo /></>",
"<React.Fragment><Foo /></React.Fragment>",
Some(json!([{"mode": "element"}])),
),
];

Tester::new(JsxFragments::NAME, JsxFragments::PLUGIN, pass, fail)
.expect_fix(fix)
.test_and_snapshot();
Expand Down
19 changes: 13 additions & 6 deletions crates/oxc_linter/src/snapshots/react_jsx_fragments.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,30 @@
---
source: crates/oxc_linter/src/tester.rs
---
⚠ eslint-plugin-react(jsx-fragments): Shorthand form for React fragments is preferred
⚠ eslint-plugin-react(jsx-fragments): Shorthand form for React fragments is preferred.
╭─[jsx_fragments.tsx:1:2]
1 │ <Fragment><Foo /></Fragment>
· ────────
╰────
help: Use <></> instead of <React.Fragment></React.Fragment>
help: Use `<></>` instead of `<React.Fragment></React.Fragment>`.

⚠ eslint-plugin-react(jsx-fragments): Shorthand form for React fragments is preferred
⚠ eslint-plugin-react(jsx-fragments): Shorthand form for React fragments is preferred.
╭─[jsx_fragments.tsx:1:2]
1 │ <React.Fragment><Foo /></React.Fragment>
· ──────────────
╰────
help: Use <></> instead of <React.Fragment></React.Fragment>
help: Use `<></>` instead of `<React.Fragment></React.Fragment>`.

⚠ eslint-plugin-react(jsx-fragments): Standard form for React fragments is preferred
⚠ eslint-plugin-react(jsx-fragments): Standard form for React fragments is preferred.
╭─[jsx_fragments.tsx:1:1]
1 │ <><Foo /></>
· ──
╰────
help: Use <React.Fragment></React.Fragment> instead of <></>
help: Use `<React.Fragment></React.Fragment>` instead of `<></>`.

⚠ eslint-plugin-react(jsx-fragments): Standard form for React fragments is preferred.
╭─[jsx_fragments.tsx:1:1]
1 │ <><Foo /></>
· ──
╰────
help: Use `<React.Fragment></React.Fragment>` instead of `<></>`.
Loading