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

[Fix] destructuring-assignment, component detection: improve component detection #3122

Merged
merged 1 commit into from Nov 5, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Expand Up @@ -21,10 +21,12 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
* [`no-unstable-components`]: improve handling of objects containing render function properties ([#3111] @fizwidget)
* [`prop-types`], `propTypes`: add forwardRef<>, ForwardRefRenderFunction<> prop-types ([#3112] @vedadeepta)
* [`no-typos`]: prevent a crash when using private methods (@ljharb)
* [`destructuring-assignment`], component detection: improve component detection ([#3122] @vedadeepta)

### Changed
* [Tests] test on the new babel eslint parser ([#3113] @ljharb)

[#3122]: https://github.com/yannickcr/eslint-plugin-react/pull/3122
[#3113]: https://github.com/yannickcr/eslint-plugin-react/pull/3113
[#3112]: https://github.com/yannickcr/eslint-plugin-react/pull/3112
[#3111]: https://github.com/yannickcr/eslint-plugin-react/pull/3111
Expand Down
15 changes: 14 additions & 1 deletion lib/util/Components.js
Expand Up @@ -334,6 +334,10 @@ function componentRule(rule, context) {
return jsxUtil.isReturningJSX(this.isCreateElement.bind(this), ASTNode, context, strict);
},

isReturningOnlyNull(ASTNode) {
return jsxUtil.isReturningOnlyNull(this.isCreateElement.bind(this), ASTNode, context);
},

getPragmaComponentWrapper(node) {
let isPragmaComponentWrapper;
let currentNode = node;
Expand Down Expand Up @@ -556,6 +560,16 @@ function componentRule(rule, context) {
return undefined;
}

// case: function any() { return (props) { return not-jsx-and-not-null } }
if (node.parent.type === 'ReturnStatement' && !utils.isReturningJSX(node) && !utils.isReturningOnlyNull(node)) {
return undefined;
}

// for case abc = { [someobject.somekey]: props => { ... return not-jsx } }
if (node.parent && node.parent.key && node.parent.key.type === 'MemberExpression' && !utils.isReturningJSX(node) && !utils.isReturningOnlyNull(node)) {
return undefined;
}

// Case like `React.memo(() => <></>)` or `React.forwardRef(...)`
const pragmaComponentWrapper = utils.getPragmaComponentWrapper(node);
if (pragmaComponentWrapper) {
Expand Down Expand Up @@ -805,7 +819,6 @@ function componentRule(rule, context) {
}

const component = utils.getParentComponent();

if (
!component
|| (component.parent && component.parent.type === 'JSXExpressionContainer')
Expand Down
48 changes: 48 additions & 0 deletions lib/util/jsx.js
Expand Up @@ -148,11 +148,59 @@ function isReturningJSX(isCreateElement, ASTnode, context, strict, ignoreNull) {
return found;
}

/**
* Check if the node is returning only null values
*
* @param {Function} isCreateElement Function to determine if a CallExpresion is
* a createElement one
* @param {ASTNode} ASTnode The AST node being checked
* @param {Context} context The context of `ASTNode`.
* @returns {Boolean} True if the node is returning only null values
*/
function isReturningOnlyNull(isCreateElement, ASTnode, context) {
let found = false;
let foundSomethingElse = false;
astUtil.traverseReturns(ASTnode, context, (node) => {
// Traverse return statement
astUtil.traverse(node, {
enter(childNode) {
const setFound = () => {
found = true;
this.skip();
};
const setFoundSomethingElse = () => {
foundSomethingElse = true;
this.skip();
};
switch (childNode.type) {
case 'ReturnStatement':
break;
case 'ConditionalExpression':
if (childNode.consequent.value === null && childNode.alternate.value === null) {
setFound();
}
break;
case 'Literal':
if (childNode.value === null) {
setFound();
}
break;
default:
setFoundSomethingElse();
}
},
});
});

return found && !foundSomethingElse;
}

module.exports = {
isDOMComponent,
isFragment,
isJSX,
isJSXAttributeKey,
isWhiteSpaces,
isReturningJSX,
isReturningOnlyNull,
};
108 changes: 108 additions & 0 deletions tests/lib/rules/destructuring-assignment.js
Expand Up @@ -20,6 +20,56 @@ const parserOptions = {
const ruleTester = new RuleTester({ parserOptions });
ruleTester.run('destructuring-assignment', rule, {
valid: parsers.all([
{
code: `
export const revisionStates2 = {
[A.b]: props => {
return props.editor !== null
? 'xyz'
: 'abc'
},
};
`,
},
{
code: `
export function hof(namespace) {
const initialState = {
bounds: null,
search: false,
};
return (props) => {
const {x, y} = props
if (y) {
return <span>{y}</span>;
}
return <span>{x}</span>
};
}
`,
},
{
code: `
export function hof(namespace) {
const initialState = {
bounds: null,
search: false,
};

return (state = initialState, action) => {
if (action.type === 'ABC') {
return {...state, bounds: stuff ? action.x : null};
}

if (action.namespace !== namespace) {
return state;
}

return null
};
}
`,
},
{
code: `
const MyComponent = ({ id, className }) => (
Expand Down Expand Up @@ -615,5 +665,63 @@ ruleTester.run('destructuring-assignment', rule, {
},
],
},
{
code: `
export const revisionStates2 = {
[A.b]: props => {
return props.editor !== null
? <span>{props.editor}</span>
: null
},
};
`,
parser: parsers.BABEL_ESLINT,
errors: [
{
messageId: 'useDestructAssignment',
data: { type: 'props' },
line: 4,
},
{
messageId: 'useDestructAssignment',
data: { type: 'props' },
line: 5,
},
],
},
{
code: `
export function hof(namespace) {
const initialState = {
bounds: null,
search: false,
};
return (props) => {
if (props.y) {
return <span>{props.y}</span>;
}
return <span>{props.x}</span>
};
}
`,
parser: parsers.BABEL_ESLINT,
errors: [
{
messageId: 'useDestructAssignment',
data: { type: 'props' },
line: 8,
},
{
messageId: 'useDestructAssignment',
data: { type: 'props' },
line: 9,
},
{
messageId: 'useDestructAssignment',
data: { type: 'props' },
line: 11,
},
],
},
]),
});