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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Fix] jsx-no-useless-fragment: accept fragments with call expressions #2744

Merged
merged 1 commit into from Aug 12, 2020
Merged

[Fix] jsx-no-useless-fragment: accept fragments with call expressions #2744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 20 additions & 6 deletions lib/rules/jsx-no-useless-fragment.js
View file
Open in desktop
Expand Up @@ -66,6 +66,17 @@ function isKeyedElement(node) {
&& node.openingElement.attributes.some(jsxUtil.isJSXAttributeKey);
}

/**
* @param {ASTNode} node
* @returns {boolean}
*/
function containsCallExpression(node) {
return node
&& node.type === 'JSXExpressionContainer'
&& node.expression
&& node.expression.type === 'CallExpression';
}

module.exports = {
meta: {
type: 'suggestion',
Expand Down Expand Up @@ -103,15 +114,18 @@ module.exports = {
* @returns {boolean}
*/
function hasLessThanTwoChildren(node) {
if (!node || !node.children || node.children.length < 2) {
if (!node || !node.children) {
return true;
}

return (
node.children.length
- (+isPaddingSpaces(node.children[0]))
- (+isPaddingSpaces(node.children[node.children.length - 1]))
) < 2;
/** @type {ASTNode[]} */
const nonPaddingChildren = node.children.filter(
(child) => !isPaddingSpaces(child)
);

if (nonPaddingChildren.length < 2) {
return !containsCallExpression(nonPaddingChildren[0]);
}
}

/**
Expand Down
10 changes: 10 additions & 0 deletions tests/lib/rules/jsx-no-useless-fragment.js
View file
Open in desktop
Expand Up @@ -63,6 +63,10 @@ ruleTester.run('jsx-no-useless-fragment', rule, {
{
code: '<Fooo content={<>eeee ee eeeeeee eeeeeeee</>} />',
parser: parsers.BABEL_ESLINT
},
{
code: '<>{foos.map(foo => foo)}</>',
Copy link
Member

Choose a reason for hiding this comment

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

this seems like a useless fragment to me tho? since foos.map produces an array, and an array is a valid render value.

Copy link
Contributor Author

@hasparus hasparus Aug 9, 2020 •
edited

Choose a reason for hiding this comment

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

In case if foos is an array, the fragment wraps an array (a React node) into a React element. It's not useless if it's intentional (e.g. a function where you pass this treats arrays different than single elements -- requires a single child or sth like that).

#2584 (comment)

But actually, we can't assume what map does, and even if map suggests an array, we treat all call expressions the same. It may return undefined and then we have a crash at runtime if render it without wrapping in the fragment.

const foos = { map: () => undefined };

Copy link
Member

Choose a reason for hiding this comment

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

What crash at runtime? A component that returns [undefined] is a valid component.

You're right that it wouldn't be useless if it's being passed somewhere, but since the introduction of fragments it seems like anything that accepts a fragment should also accept an array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What crash at runtime? A component that returns [undefined] is a valid component.

Yes, but it's not valid if it returns undefined. There is no array here.

This doesn't have to be map, but even when the function is called "map" we have no guarantee that this is Array.prototype.map. A function may return undefined.

parser: parsers.BABEL_ESLINT
}
],
invalid: [
Expand All @@ -72,6 +76,12 @@ ruleTester.run('jsx-no-useless-fragment', rule, {
errors: [{messageId: 'NeedsMoreChidren', type: 'JSXFragment'}],
parser: parsers.BABEL_ESLINT
},
{
code: '<>{}</>',
ljharb marked this conversation as resolved.
Show resolved Hide resolved
output: null,
errors: [{messageId: 'NeedsMoreChidren', type: 'JSXFragment'}],
parser: parsers.BABEL_ESLINT
},
{
code: '<p>moo<>foo</></p>',
output: '<p>moofoo</p>',
Expand Down