-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[New] add no-render-return-undefined
: disallow components rendering undefined
#3750
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
base: master
Are you sure you want to change the base?
Changes from all commits
499e261
86d7ac8
61981c7
34b6a94
56e7436
9202a30
639f15f
8aec646
d1814c1
7905adf
b7ca1dd
f4983ae
0b56b09
94e9817
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
# Disallow returning undefined from react components (`react/no-render-return-undefined`) | ||
|
||
💼 This rule is enabled in the ☑️ `recommended` [config](https://github.com/jsx-eslint/eslint-plugin-react/#shareable-configs). | ||
|
||
<!-- end auto-generated rule header --> | ||
|
||
> 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`. | ||
|
||
## Rule Details | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. let's add one that uses There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ping |
||
|
||
```jsx | ||
function App() {} | ||
|
||
// OR | ||
|
||
function App() { | ||
return undefined; | ||
} | ||
|
||
// OR | ||
|
||
let ui; | ||
function App() { | ||
return ui; | ||
} | ||
|
||
// OR | ||
|
||
function getUI() { | ||
if (condition) return <h1>Hello</h1>; | ||
} | ||
function App() { | ||
return getUI(); | ||
} | ||
Comment on lines
+33
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this won't be possible across files, only if it's in the same file, so unless the function provides return type information (ie, TS) i don't think this is a check we can reliably do. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to continue without supporting this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ljharb Can we do something here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I don't think we necessarily have to support this - I'm saying that the docs should be updated to reflect the caveats. |
||
``` | ||
|
||
Examples of **correct** code for this rule: | ||
|
||
```jsx | ||
function App() { | ||
return <div />; | ||
} | ||
|
||
// OR | ||
|
||
let ui = <div />; | ||
function App() { | ||
return ui; | ||
} | ||
|
||
// OR | ||
|
||
function getUI() { | ||
if (condition) return <h1>Hello</h1>; | ||
return null; | ||
} | ||
function App() { | ||
return getUI(); | ||
} | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,157 @@ | ||
/** | ||
* @fileoverview Prevent returning undefined from react components | ||
* @author Akul Srivastava | ||
*/ | ||
|
||
'use strict'; | ||
|
||
const astUtil = require('../util/ast'); | ||
const docsUrl = require('../util/docsUrl'); | ||
const isFirstLetterCapitalized = require('../util/isFirstLetterCapitalized'); | ||
const report = require('../util/report'); | ||
const variableUtil = require('../util/variable'); | ||
|
||
const messages = { | ||
returnsUndefined: "Don't return `undefined` from react components", | ||
}; | ||
|
||
function getReturnValue(context, returnNode) { | ||
const variables = variableUtil.variablesInScope(context, returnNode); | ||
const returnIdentifierName = returnNode && returnNode.name; | ||
const returnIdentifierVar = variableUtil.getVariable( | ||
variables, | ||
returnIdentifierName | ||
); | ||
|
||
if (!returnNode) return undefined; | ||
|
||
if ( | ||
returnIdentifierVar | ||
&& returnIdentifierVar.defs | ||
&& returnIdentifierVar.defs[0] | ||
) { | ||
const value = returnIdentifierVar.defs[0].node.init; | ||
if ( | ||
returnIdentifierVar.defs[0].node | ||
Comment on lines
+33
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if line 35 is nullish, then line 33 will throw - i think this needs to be checked before accessing |
||
&& returnIdentifierVar.defs[0].node.type === 'VariableDeclarator' | ||
&& value === null | ||
) { | ||
return undefined; | ||
} | ||
return value; | ||
} | ||
|
||
switch (returnNode.type) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's use an if/else here instead of a switch |
||
case 'LogicalExpression': { | ||
return getReturnValue(context, returnNode.right); | ||
} | ||
case 'ConditionalExpression': { | ||
const possibleReturnValue = [ | ||
getReturnValue(context, returnNode.consequent), | ||
getReturnValue(context, returnNode.alternate), | ||
]; | ||
const returnsUndefined = possibleReturnValue.some((val) => typeof val === 'undefined'); | ||
if (returnsUndefined) return; | ||
return possibleReturnValue; | ||
} | ||
case 'CallExpression': { | ||
if (returnNode.callee.type === 'MemberExpression') { | ||
const calleeObjName = returnNode.callee.object.name; | ||
const calleePropertyName = returnNode.callee.property.name; | ||
const calleeObjNode = variables.find((item) => item && item.name === calleeObjName); | ||
const isCalleeObjArray = calleeObjNode.defs[0].node.init.type === 'ArrayExpression'; | ||
const isMapCall = isCalleeObjArray && calleePropertyName === 'map'; | ||
if (isMapCall) { | ||
const mapCallback = returnNode.arguments[0]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a test for an SFC that omits |
||
const mapReturnStatement = mapCallback.body.type === 'BlockStatement' | ||
&& astUtil.findReturnStatement(returnNode.arguments[0]); | ||
const mapReturnNode = (mapReturnStatement && mapReturnStatement.argument) || mapCallback.body; | ||
// console.log('DEBUG', mapReturnNode); | ||
return getReturnValue(context, mapReturnNode); | ||
} | ||
} | ||
const calleeName = returnNode.callee.name; | ||
const calleeNode = variables.find((item) => item && item.name === calleeName); | ||
const calleeDefinitionNode = calleeNode && calleeNode.defs && calleeNode.defs[0] && calleeNode.defs[0].node; | ||
const calleeReturnStatement = astUtil.findReturnStatement(calleeDefinitionNode); | ||
const calleeReturnNode = (calleeReturnStatement && calleeReturnStatement.argument) | ||
|| (calleeDefinitionNode.init && calleeDefinitionNode.init.body); | ||
return getReturnValue(context, calleeReturnNode); | ||
} | ||
case 'ArrayExpression': { | ||
return returnNode.elements; | ||
} | ||
case 'JSXElement': { | ||
return returnNode; | ||
} | ||
default: | ||
return returnNode.value; | ||
} | ||
} | ||
|
||
/** @type {import('eslint').Rule.RuleModule} */ | ||
module.exports = { | ||
meta: { | ||
docs: { | ||
description: 'Disallow returning `undefined` from react components', | ||
category: 'Best Practices', | ||
recommended: false, | ||
url: docsUrl('no-render-return-undefined'), | ||
}, | ||
messages, | ||
schema: [], | ||
}, | ||
|
||
create(context) { | ||
const isReturningUndefined = (returnStatement) => { | ||
const returnNode = returnStatement && returnStatement.argument; | ||
const returnIdentifierName = returnNode && returnNode.name; | ||
|
||
const returnIdentifierValue = getReturnValue(context, returnNode); | ||
|
||
const returnsArrayHavingUndefined = Array.isArray(returnIdentifierValue) | ||
&& returnIdentifierValue.some((el) => el && el.type === 'Identifier' && el.name === 'undefined'); | ||
|
||
return !returnStatement | ||
|| returnIdentifierName === 'undefined' | ||
|| typeof returnIdentifierValue === 'undefined' | ||
|| (returnIdentifierValue && returnIdentifierValue.name === 'undefined') | ||
|| returnsArrayHavingUndefined; | ||
}; | ||
|
||
const handleFunctionalComponents = (node) => { | ||
const fnName = (node.id && node.id.name) || (node.parent.id && node.parent.id.name); | ||
|
||
// Considering functions starting with Uppercase letters are react components | ||
const isReactComponent = isFirstLetterCapitalized(fnName); | ||
const returnStatement = astUtil.findReturnStatement(node); | ||
|
||
if (!isReactComponent) return; | ||
|
||
if (isReturningUndefined(returnStatement)) { | ||
report(context, messages.returnsUndefined, 'returnsUndefined', { | ||
node, | ||
}); | ||
} | ||
}; | ||
|
||
const handleClassComponents = (node) => { | ||
const componentProperties = astUtil.getComponentProperties(node); | ||
const renderFnNode = componentProperties.find((prop) => prop.key.name === 'render'); | ||
const returnStatement = astUtil.findReturnStatement(renderFnNode); | ||
|
||
if (isReturningUndefined(returnStatement)) { | ||
report(context, messages.returnsUndefined, 'returnsUndefined', { | ||
node, | ||
}); | ||
} | ||
}; | ||
|
||
return { | ||
FunctionDeclaration: handleFunctionalComponents, | ||
ArrowFunctionExpression: handleFunctionalComponents, | ||
ClassDeclaration: handleClassComponents, | ||
ClassExpression: handleClassComponents, | ||
}; | ||
}, | ||
}; |
Uh oh!
There was an error while loading. Please reload this page.