Merged
Conversation
|
Size Change: 0 B Total Size: 8.76 MB ℹ️ View Unchanged
|
|
Flaky tests detected in b06a60f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/23267687249
|
# Conflicts: # packages/eslint-plugin/CHANGELOG.md
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Member
Author
|
Confirmed that there are no deprecated Eslint APIs being used (#76507). |
ciampo
reviewed
Mar 17, 2026
Comment on lines
+20
to
+25
| ```jsx | ||
| // className destructured but not included in the className expression | ||
| function Foo( { className, ...restProps } ) { | ||
| return <div className={ clsx( styles.foo ) } { ...restProps } />; | ||
| } | ||
| ``` |
Contributor
There was a problem hiding this comment.
This same example is listed as valid in the tests ("className destructured but not referenced — not this rule's concern") — I feel like the rule should flag it as invalid?
Member
Author
There was a problem hiding this comment.
Good catch on the inconsistency. The docs were wrong — the rule intentionally doesn't flag this case. If the rule also checked whether className appears in the expression, it would false-positive on the multi-element pattern where className is forwarded to a different element than the one with the spread (this pattern actually exists in the package):
function Foo( { className, style, ...props } ) {
return (
<Outer className={ clsx( styles.outer, className ) } style={ style }>
<Inner className={ styles.inner } { ...props } />
</Outer>
);
}So this is basically an intentional decision to balance complexity and practical utility. We can assume that a destructured className and no no-unused-vars error means that that className was used somewhere.
I've updated the docs to remove that example from "incorrect" and added this forwarding pattern as an explicit "correct" example (b06a60f).
Contributor
There was a problem hiding this comment.
If I undertand the code, this rule will flag a false positive when props are destructured in the render function's body, eg
function Foo( { ...restProps } ) {
const { className, ...other } = restProps;
return <div className={ clsx( styles.foo, className ) } { ...other } />;
}although I guess it's rare edge case?
Member
Author
There was a problem hiding this comment.
Correct, that's a known limitation. In practice this pattern doesn't come up in @wordpress/ui, so I think it's fine to leave as-is for now rather than adding the complexity of tracking variable assignments through the function body.
Contributor
|
Very nitty, but seeing #76611, should we also add TS-specific tests for this rule, to make sure that it correctly handles typed parameters? Opus suggests the following// TypeScript-specific tests — verify the rule works with typed parameters,
// which is the norm in packages/ui/src/.
const tsRuleTester = new RuleTester( {
parser: require.resolve( '@typescript-eslint/parser' ),
parserOptions: {
ecmaVersion: 2020,
sourceType: 'module',
ecmaFeatures: { jsx: true },
},
} );
tsRuleTester.run( 'no-unmerged-classname (TypeScript)', rule, {
valid: [
{
// Typed destructured props with className — the standard @wordpress/ui pattern
code: `
function Foo( { className, ...restProps }: FooProps ) {
return <div className={ clsx( styles.foo, className ) } { ...restProps } />;
}
`,
},
{
// forwardRef with typed props
code: `
const Foo = forwardRef< HTMLDivElement, FooProps >(
function Foo( { className, ...restProps }: FooProps, ref ) {
return <div ref={ ref } className={ clsx( styles.foo, className ) } { ...restProps } />;
}
);
`,
},
],
invalid: [
{
// Typed props without className destructured
code: `
function Foo( { padding, ...restProps }: FooProps ) {
return <div className={ styles.foo } { ...restProps } />;
}
`,
errors: [ { messageId: 'noUnmergedClassname' } ],
},
{
// forwardRef with typed props, className not destructured
code: `
const Foo = forwardRef< HTMLDivElement, FooProps >(
function Foo( { padding, ...restProps }: FooProps, ref ) {
return <div ref={ ref } className={ styles.foo } { ...restProps } />;
}
);
`,
errors: [ { messageId: 'noUnmergedClassname' } ],
},
],
} ); |
Member
Author
I looked into this since I wasn't familiar, but short answer is no. Our rule here doesn't have any TS-specific logic. It only inspects standard JS AST nodes which both parsers produce identically regardless of type annotations (unlike the |
adamsilverstein
pushed a commit
that referenced
this pull request
Mar 31, 2026
* ESLint: Add `no-unmerged-classname` rule * Enable `no-unmerged-classname` for `@wordpress/ui` * Add changelog * Fix docs to match rule behavior Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Adds a custom ESLint rule,
@wordpress/no-unmerged-classname, and enables it forpackages/ui/src/.Why?
In
@wordpress/ui, components that spread rest props onto a JSX element can silently drop the consumer-suppliedclassNameif it isn't properly merged. For example:Here, any
classNamepassed by the consumer is inrestProps, but the explicitclassNameafter the spread silently overwrites it. The correct pattern is to destructureclassNameand merge it (e.g. viaclsx).This is a subtle mistake — the code looks reasonable and TypeScript won't flag it. A lint rule catches it at authoring time.
How?
The rule flags any JSX element that has both an explicit
classNameattribute and a spread attribute, where the enclosing function's props parameter doesn't destructureclassName. It handles:{ padding, ...rest }) — flags ifclassNameis not among the destructured properties.props) — flags if the identifier is spread directly onto the element.classNameto a different element than the one with the spread (e.g. the Tooltip pattern) — not flagged, sinceclassNameis destructured.The rule is not included in any preset. It's enabled for
packages/ui/src/**only, excluding test and story files.Testing Instructions
npm run test:unit -- --testPathPattern='packages/eslint-plugin/rules/__tests__/no-unmerged-classname'