jsx-eslint/eslint-plugin-reactPublic

NotificationsYou must be signed in to change notification settings
Fork2.8k
Star9.2k

[Fix]`display-name`: avoid false positive when React is shadowed#3926

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

hyeonbinHur wants to merge7 commits intojsx-eslint:master

base:master

Choose a base branch

fromhyeonbinHur:issue3924

Open

[Fix]`display-name`: avoid false positive when React is shadowed#3926

hyeonbinHur wants to merge7 commits intojsx-eslint:masterfromhyeonbinHur:issue3924

Conversation

Copy link

hyeonbinHur commentedMay 22, 2025

Fixes#3924

[Fix]display-name: avoid false positive when React is shadowed

30f0022

Fixesjsx-eslint#3924

Copy link

codecovbot commentedMay 22, 2025•
edited
Loading

Codecov Report

Attention: Patch coverage is82.53968% with11 lines in your changes missing coverage. Please review.

Project coverage is 97.75%. Comparing base(dd2e968) to head(f6800d3).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
lib/rules/display-name.js	82.53%	11 Missing⚠️

Additional details and impacted files

@@            Coverage Diff             @@##           master    #3926      +/-   ##==========================================+ Coverage   97.73%   97.75%   +0.01%==========================================  Files         136      136                Lines        9997    10058      +61       Branches     3715     3750      +35     ==========================================+ Hits         9771     9832      +61  Misses        226      226

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report?Share it here.

🚀 New features to boost your workflow:

❄️Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ljharb requested changes

May 22, 2025

View reviewed changes

Copy link

Member

ljharb left a comment

Choose a reason for hiding this comment

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

can't review til style diffs are reverted

lib/rules/display-name.js Outdated

Comment on lines 34 to 35

		description:
		'Disallow missing displayName in a React component definition',

Copy link

Member

Choose a reason for hiding this comment

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

please don't run prettier on projects that don't use it :-) and please revert all unrelated style changes.

Copy link

Author

Choose a reason for hiding this comment

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

Sorry, I forgot to disable some style-related settings, which caused unnecessary formatting changes. I’ll remove the unnecessary style edits and commit again soon. Thanks for the review and the feedback!

Copy link

Member

ljharbMay 22, 2025

Choose a reason for hiding this comment

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

My suggestion is to keep those always disabled, and only use eslint to format files.

Copy link

morganneyMay 22, 2025

Choose a reason for hiding this comment

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

Excepteslint recommends to use formatters instead.

ljharb marked this pull request as draft

May 22, 2025 18:42

style: fix lint,prettier errors

6e05536

ljharb reviewed

May 22, 2025

View reviewed changes

lib/rules/display-name.js OutdatedShow resolvedHide resolved

tests/lib/rules/display-name.js OutdatedShow resolvedHide resolved

tests/lib/rules/display-name.js Outdated

Comment on lines 46 to 57

		code: `
		import React, { memo } from 'react'

		const TestComponent = function () {
		const memo = (cb) => cb()

		const Comp = memo(() => {
		return <div>shadowed</div>
		})

		return Comp
		}

Copy link

Member

ljharbMay 22, 2025

Choose a reason for hiding this comment

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

can we get every "valid" example with shadowing, also added as an invalid example without the shadowing?

morganney reviewed

May 22, 2025

View reviewed changes

lib/rules/display-name.js

Comment on lines +206 to +208

		currentNode.type === 'FunctionDeclaration'
		\|\| currentNode.type === 'FunctionExpression'
		\|\| currentNode.type === 'ArrowFunctionExpression'

Copy link

morganneyMay 22, 2025

Choose a reason for hiding this comment

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

Why only these scopes? For example what aboutBlockStatement:

{  const memo = (cb) => cb()  const forwardRef = (cb) => cb()  const React = { memo, forwardRef }  const BlockReactShadowedMemo = React.memo(() => {    return <div>shadowed</div>  })}

Copy link

Author

hyeonbinHurMay 23, 2025

Choose a reason for hiding this comment

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

Thank you for the review. Let me take a quick look and try to find a better algorithm to cover all test cases.

hyeonbinHurand others added4 commits

May 23, 2025 15:04

Update lib/rules/display-name.js

16e8aef

Co-authored-by: Jordan Harband <ljharb@gmail.com>

Update tests/lib/rules/display-name.js

88092cd

Co-authored-by: Jordan Harband <ljharb@gmail.com>

fix(display-name): expand shadowing test coverage and functionalities

48491b9

fix(display-name): optimize shadowing test cases

513cdf9

Copy link

Author

hyeonbinHur commentedMay 26, 2025

Valid Test Cases (4)

Case	Secenario	Shadowing Patterns Covered
1: BlockStatement	All React APIs shadowed within block scope	• Block scope variables • { const memo = ... } • { const React = ... }
2: Function Parameters	Shadowing via function parameters	• Regular parameters: function(memo) • Destructured parameters: function({forwardRef})
3: Nested Function Scope	Shadowing within nested functions	• Nested function scope • Inner function variables • function inner() { const memo = ... }
4: Mixed Patterns	Multiple shadowing techniques combined	• Variable declaration: const memo = ... • Destructuring: const {forwardRef} = ... • Array destructuring: const [React] = ...

Invalid Test Cases (3)

Case	Scenario	Corresponding Valid Case
1: BlockStatement Errors	Direct React API usage in block scope	Valid 1
2: Parameter + Nested Errors	Direct React API usage without parameter/nested shadowing	Valid 2, 3
3: Mixed Pattern Errors	Direct React API usage without any shadowing	Valid 4

Update display-name.js

f6800d3

hyeonbinHur marked this pull request as ready for review

May 26, 2025 18:55

Labels

None yet

3 participants

Movatterモバイル変換

Uh oh!

[Fix]display-name: avoid false positive when React is shadowed#3926

Are you sure you want to change the base?

[Fix]display-name: avoid false positive when React is shadowed#3926

Uh oh!

Conversation

hyeonbinHur commentedMay 22, 2025

Uh oh!

codecovbot commentedMay 22, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

ljharbMay 22, 2025

Choose a reason for hiding this comment

Uh oh!

hyeonbinHurMay 22, 2025

Choose a reason for hiding this comment

Uh oh!

ljharbMay 22, 2025

Choose a reason for hiding this comment

Uh oh!

morganneyMay 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ljharbMay 22, 2025

Choose a reason for hiding this comment

Uh oh!

morganneyMay 22, 2025

Choose a reason for hiding this comment

Uh oh!

hyeonbinHurMay 23, 2025

Choose a reason for hiding this comment

Uh oh!

hyeonbinHur commentedMay 26, 2025

Uh oh!

Uh oh!

[Fix]`display-name`: avoid false positive when React is shadowed#3926

[Fix]`display-name`: avoid false positive when React is shadowed#3926

codecovbot commentedMay 22, 2025•
edited
Loading