-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[New] add no-render-return-undefined
: disallow components rendering undefined
#3750
base: master
Are you sure you want to change the base?
[New] add no-render-return-undefined
: disallow components rendering undefined
#3750
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3750 +/- ##
==========================================
- Coverage 97.62% 94.45% -3.18%
==========================================
Files 134 135 +1
Lines 9617 9708 +91
Branches 3488 3535 +47
==========================================
- Hits 9389 9170 -219
- Misses 228 538 +310 ☔ View full report in Codecov by Sentry. |
@ljharb Let me know your thoughts on this! |
|
||
<!-- end auto-generated rule header --> | ||
|
||
> In React 18, components may render undefined, and React will render nothing to the DOM instead of throwing an error. However, accidentally rendering nothing in a component could still cause surprises. This rule will warn if the `return` statement in a React Component returns undefined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
> In React 18, components may render undefined, and React will render nothing to the DOM instead of throwing an error. However, accidentally rendering nothing in a component could still cause surprises. This rule will warn if the `return` statement in a React Component returns undefined. | |
> Starting in React 18, components may render `undefined`, and React will render nothing to the DOM instead of throwing an error. However, accidentally rendering nothing in a component could still cause surprises. This rule will warn if the `return` statement in a React Component returns `undefined`. |
|
||
> In React 18, components may render undefined, and React will render nothing to the DOM instead of throwing an error. However, accidentally rendering nothing in a component could still cause surprises. This rule will warn if the `return` statement in a React Component returns undefined. | ||
|
||
Issue: [#3020](https://github.com/jsx-eslint/eslint-plugin-react/issues/3020) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure linking to the issue is needed
|
||
## Rule Details | ||
|
||
This rule will warn if the `return` statement in a React component returns undefined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rule will warn if the `return` statement in a React component returns undefined. | |
This rule will warn if the `return` statement in a React component returns `undefined`. |
|
||
This rule will warn if the `return` statement in a React component returns undefined. | ||
|
||
Examples of **incorrect** code for this rule: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add one that uses void
}, | ||
|
||
create(context) { | ||
function getReturnValue(returnNode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this take context
and be defined at module level instead?
} | ||
case 'ConditionalExpression': { | ||
const possibleReturnValue = [getReturnValue(returnNode.consequent), getReturnValue(returnNode.alternate)]; | ||
const returnsUndefined = possibleReturnValue.some((val) => val === undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const returnsUndefined = possibleReturnValue.some((val) => val === undefined); | |
const returnsUndefined = possibleReturnValue.some((val) => typeof val === 'undefined'); |
case 'ConditionalExpression': { | ||
const possibleReturnValue = [getReturnValue(returnNode.consequent), getReturnValue(returnNode.alternate)]; | ||
const returnsUndefined = possibleReturnValue.some((val) => val === undefined); | ||
if (returnsUndefined) return undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (returnsUndefined) return undefined; | |
if (returnsUndefined) return; |
|
||
return !returnStatement | ||
|| returnIdentifierName === 'undefined' | ||
|| returnIdentifierValue === undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|| returnIdentifierValue === undefined | |
|| typeof returnIdentifierValue === 'undefined' |
package.json
Outdated
@@ -15,7 +15,7 @@ | |||
"test": "npm run unit-test", | |||
"posttest": "aud --production", | |||
"type-check": "tsc", | |||
"unit-test": "istanbul cover node_modules/mocha/bin/_mocha tests/lib/**/*.js tests/util/**/*.js tests/index.js", | |||
"unit-test": "istanbul cover node_modules/mocha/bin/_mocha tests/lib/rules/no-render-return-undefined.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commenting here to ensure this doesn't get merged. fwiw you can npx mocha tests/lib/rules/no-render-return-undefined.js
to run just that one file locally :-)
6d3cfec
to
d1814c1
Compare
I've pushed the review changes |
Bumping this up! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's still some unaddressed comments on the docs.
lib/rules/index.js
Outdated
@@ -105,4 +105,5 @@ module.exports = { | |||
'static-property-placement': require('./static-property-placement'), | |||
'style-prop-object': require('./style-prop-object'), | |||
'void-dom-elements-no-children': require('./void-dom-elements-no-children'), | |||
'no-render-return-undefined': require('./no-render-return-undefined'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's sort this alphabetically
no-render-return-undefined
: disallow components rendering undefinedno-render-return-undefined
: disallow components rendering undefined
Closes #3020