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.

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

Enforce JSX inline conditional as a ternary #3318

Closed
wants to merge 1 commit into from

Conversation

holic
Copy link

@holic holic commented Jul 2, 2022

This fixes a fairly common bug where React will render the falsy left side of a conditional with && operator, resulting in things like 0 showing up in your UI. See #2888 (comment).

Fixes #1979
Closes #2888

cc @ljharb

parserOptions: {
ecmaFeatures: {
jsx: true,
},
},
rules: {
'react/display-name': 2,
'react/jsx-inline-conditional': 2,
Copy link
Author

Choose a reason for hiding this comment

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

Because this is helps identify+fix a certain class of rendering bugs and isn't strictly stylistic, I enabled this by default. Let me know if I should change this!

Copy link
Member

Choose a reason for hiding this comment

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

Adding a rule to the recommended config is a breaking change, so we'll basically never be doing that. Please remove it.

@codecov
Copy link

codecov bot commented Jul 2, 2022

Codecov Report

Merging #3318 (6e6e2db) into master (aac7fb9) will decrease coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3318      +/-   ##
==========================================
- Coverage   97.70%   97.57%   -0.14%     
==========================================
  Files         123      124       +1     
  Lines        8764     8777      +13     
  Branches     3176     3180       +4     
==========================================
+ Hits         8563     8564       +1     
- Misses        201      213      +12     
Impacted Files Coverage Δ
index.js 100.00% <ø> (ø)
lib/rules/jsx-inline-conditional.js 100.00% <100.00%> (ø)
lib/rules/no-arrow-function-lifecycle.js 88.52% <0.00%> (-9.84%) ⬇️
lib/rules/jsx-curly-spacing.js 91.17% <0.00%> (-2.95%) ⬇️
lib/rules/jsx-props-no-spreading.js 95.65% <0.00%> (-2.18%) ⬇️
lib/util/componentUtil.js 97.33% <0.00%> (-1.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aac7fb9...6e6e2db. Read the comment docs.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Doesn't this duplicate the recently added jsx-no-leaked-render rule?

parserOptions: {
ecmaFeatures: {
jsx: true,
},
},
rules: {
'react/display-name': 2,
'react/jsx-inline-conditional': 2,
Copy link
Member

Choose a reason for hiding this comment

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

Adding a rule to the recommended config is a breaking change, so we'll basically never be doing that. Please remove it.

plugins: [
'react',
],
plugins: ['react'],
Copy link
Member

Choose a reason for hiding this comment

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

please revert unrelated formatting changes; if perhaps you're running prettier on repos that do not use it, please don't do that either :-)

Comment on lines +46 to +53
{
code: '<div>{possiblyNull ?? <SomeComponent />}</div>',
parser: parsers.TYPESCRIPT_ESLINT,
},
{
code: '<div>{possiblyNull ?? <SomeComponent />}</div>',
parser: parsers['@TYPESCRIPT_ESLINT'],
},
Copy link
Member

Choose a reason for hiding this comment

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

these should not be needed, since the parsers.all helper should automatically run every test case in every parser.

Suggested change
{
code: '<div>{possiblyNull ?? <SomeComponent />}</div>',
parser: parsers.TYPESCRIPT_ESLINT,
},
{
code: '<div>{possiblyNull ?? <SomeComponent />}</div>',
parser: parsers['@TYPESCRIPT_ESLINT'],
},

Comment on lines +41 to +44
code: '<div>{possiblyNull ?? <SomeComponent />}</div>',
parserOptions: {
ecmaVersion: 2020,
},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
code: '<div>{possiblyNull ?? <SomeComponent />}</div>',
parserOptions: {
ecmaVersion: 2020,
},
code: '<div>{possiblyNull ?? <SomeComponent />}</div>',
features: ['nullish coalescing'],

and then we'll need to add that feature to the parsers.all helper

@ljharb
Copy link
Member

ljharb commented Aug 13, 2022

I think #3203 may have covered this?

@ljharb ljharb marked this pull request as draft August 13, 2022 21:32
@holic holic closed this Aug 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

prohibit && conditional rendering in the return statement of a component
2 participants