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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[fix]jsx-curly-brace-presence: report unnecessary curly braces on multline child expressions #2409

Merged
merged 2 commits into from Sep 13, 2019
Merged

[fix]jsx-curly-brace-presence: report unnecessary curly braces on multline child expressions #2409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b2db61
[fix] `jsx-curly-brace-presence`: report unnecessary curly braces wit…
vedadeepta Sep 12, 2019
489ced3
[refactor] `jsx-curly-braces-presence, jsx-one-expression-per-line, n…
vedadeepta Sep 13, 2019
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
41 changes: 34 additions & 7 deletions lib/rules/jsx-curly-brace-presence.js
View file
Open in desktop
Expand Up @@ -104,7 +104,7 @@ module.exports = {
function containsWhitespaceExpression(child) {
if (child.type === 'JSXExpressionContainer') {
const value = child.expression.value;
return value ? !(/\S/.test(value)) : false;
return value ? jsxUtil.isWhiteSpaces(value) : false;
}
return false;
}
Expand Down Expand Up @@ -205,17 +205,44 @@ module.exports = {
);
}

function shouldCheckForUnnecessaryCurly(parent, config) {
// If there are more than one JSX child, there is no need to check for
// unnecessary curly braces.
if (jsxUtil.isJSX(parent) && parent.children.length !== 1) {
function isWhiteSpaceLiteral(node) {
vedadeepta marked this conversation as resolved.
Show resolved Hide resolved
return node.type && node.type === 'Literal' && node.value && jsxUtil.isWhiteSpaces(node.value);
}

function getAdjacentSiblings(node, children) {
for (let i = 1; i < children.length - 1; i++) {
const child = children[i];
if (node === child) {
return [children[i - 1], children[i + 1]];
}
}
if (node === children[0] && children[1]) {
return [children[1]];
}
if (node === children[children.length - 1] && children[children.length - 2]) {
return [children[children.length - 2]];
}
return [];
}

function hasAdjacentJsxExpressionContainers(node, children) {
const childrenExcludingWhitespaceLiteral = children.filter(child => !isWhiteSpaceLiteral(child));
const adjSiblings = getAdjacentSiblings(node, childrenExcludingWhitespaceLiteral);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this check is quite right. We now get stuff like:

           <div>
-            Confirm{' '}
+            Confirm
             <button type="button" onClick={this.onClickYes}>

But this is not the same markup and it's not desirable here.


return adjSiblings.some(x => x.type && x.type === 'JSXExpressionContainer');
}

function shouldCheckForUnnecessaryCurly(parent, node, config) {
// If there are adjacent `JsxExpressionContainer` then there is no need,
// to check for unnecessary curly braces.
if (jsxUtil.isJSX(parent) && hasAdjacentJsxExpressionContainers(node, parent.children)) {
return false;
}

if (
parent.children &&
parent.children.length === 1 &&
containsWhitespaceExpression(parent.children[0])
containsWhitespaceExpression(node)
) {
return false;
}
Expand All @@ -241,7 +268,7 @@ module.exports = {

return {
JSXExpressionContainer: (node) => {
if (shouldCheckForUnnecessaryCurly(node.parent, userConfig)) {
if (shouldCheckForUnnecessaryCurly(node.parent, node, userConfig)) {
lintUnnecessaryCurly(node);
}
},
Expand Down
3 changes: 2 additions & 1 deletion lib/rules/jsx-one-expression-per-line.js
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@
'use strict';

const docsUrl = require('../util/docsUrl');
const jsxUtil = require('../util/jsx');

// ------------------------------------------------------------------------------
// Rule Definition
Expand Down Expand Up @@ -89,7 +90,7 @@ module.exports = {
let countNewLinesAfterContent = 0;

if (child.type === 'Literal' || child.type === 'JSXText') {
if (/^\s*$/.test(child.raw)) {
if (jsxUtil.isWhiteSpaces(child.raw)) {
return;
}

Expand Down
3 changes: 2 additions & 1 deletion lib/rules/no-danger-with-children.js
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@
'use strict';

const variableUtil = require('../util/variable');
const jsxUtil = require('../util/jsx');
const docsUrl = require('../util/docsUrl');

// ------------------------------------------------------------------------------
Expand Down Expand Up @@ -81,7 +82,7 @@ module.exports = {
function isLineBreak(node) {
const isLiteral = node.type === 'Literal' || node.type === 'JSXText';
const isMultiline = node.loc.start.line !== node.loc.end.line;
const isWhiteSpaces = /^\s*$/.test(node.value);
const isWhiteSpaces = jsxUtil.isWhiteSpaces(node.value);

return isLiteral && isMultiline && isWhiteSpaces;
}
Expand Down
12 changes: 11 additions & 1 deletion lib/util/jsx.js
View file
Open in desktop
Expand Up @@ -76,9 +76,19 @@ function isJSXAttributeKey(node) {
node.name.name === 'key';
}

/**
* Check if value has only whitespaces
* @param {string} value
* @returns {boolean}
*/
function isWhiteSpaces(value) {
return typeof value === 'string' ? /^\s*$/.test(value) : false;
}

module.exports = {
isDOMComponent,
isFragment,
isJSX,
isJSXAttributeKey
isJSXAttributeKey,
isWhiteSpaces
};
86 changes: 86 additions & 0 deletions tests/lib/rules/jsx-curly-brace-presence.js
View file
Open in desktop
Expand Up @@ -284,6 +284,25 @@ ruleTester.run('jsx-curly-brace-presence', rule, {
\`}</App>
`,
options: ['always']
},
{
code: `
<MyComponent>
%
</MyComponent>
`,
parser: parsers.BABEL_ESLINT,
options: [{children: 'never'}]
},
{
code: `
<MyComponent>
foo
<div>bar</div>
</MyComponent>
`,
parser: parsers.BABEL_ESLINT,
options: [{children: 'never'}]
}
],

Expand Down Expand Up @@ -335,6 +354,73 @@ ruleTester.run('jsx-curly-brace-presence', rule, {
options: [{props: 'never'}],
errors: [{message: unnecessaryCurlyMessage}]
},
{
code: `
<MyComponent>
{'%'}
</MyComponent>
`,
output: `
<MyComponent>
%
</MyComponent>
`,
parser: parsers.BABEL_ESLINT,
options: [{children: 'never'}],
errors: [{message: unnecessaryCurlyMessage}]
},
{
code: `
<MyComponent>
{'foo'}
<div>
{'bar'}
</div>
{'baz'}
</MyComponent>
`,
output: `
<MyComponent>
foo
<div>
bar
</div>
baz
</MyComponent>
`,
parser: parsers.BABEL_ESLINT,
options: [{children: 'never'}],
errors: [
{message: unnecessaryCurlyMessage},
{message: unnecessaryCurlyMessage},
{message: unnecessaryCurlyMessage}
]
},
{
code: `
<MyComponent>
{'foo'}
vedadeepta marked this conversation as resolved.
Show resolved Hide resolved
<div>
{'bar'}
</div>
{'baz'}
{'some-complicated-exp'}
</MyComponent>
`,
output: `
<MyComponent>
foo
<div>
bar
</div>
{'baz'}
{'some-complicated-exp'}
</MyComponent>
`,
parser: parsers.BABEL_ESLINT,
options: [{children: 'never'}],
errors: [{message: unnecessaryCurlyMessage}, {message: unnecessaryCurlyMessage}]
},
{
code: `<MyComponent prop='bar'>foo</MyComponent>`,
output: '<MyComponent prop={"bar"}>foo</MyComponent>',
Expand Down