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

[New] add no-render-return-undefined: disallow components rendering undefined #3750

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from
Draft
64 changes: 64 additions & 0 deletions docs/rules/no-render-return-undefined.md
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).
ljharb marked this conversation as resolved.
Show resolved Hide resolved

<!-- 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:
Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible to continue without supporting this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ljharb Can we do something here?
Rest all changes, I've made

Copy link
Member

Choose a reason for hiding this comment

The 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();
}
```
1 change: 1 addition & 0 deletions lib/rules/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
ljharb marked this conversation as resolved.
Show resolved Hide resolved
};
157 changes: 157 additions & 0 deletions lib/rules/no-render-return-undefined.js
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
Copy link
Member

Choose a reason for hiding this comment

The 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 .init

&& returnIdentifierVar.defs[0].node.type === 'VariableDeclarator'
&& value === null
) {
return undefined;
}
return value;
}

switch (returnNode.type) {
Copy link
Member

Choose a reason for hiding this comment

The 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];
Copy link
Member

Choose a reason for hiding this comment

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

can you add a test for an SFC that omits return entirely?

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,
};
},
};
2 changes: 1 addition & 1 deletion lib/util/eslint.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ function getAncestors(context, node) {

function getScope(context, node) {
const sourceCode = getSourceCode(context);
if (sourceCode.getScope) {
if (sourceCode.getScope && node) {
return sourceCode.getScope(node);
}

Expand Down
Loading
Loading