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

[jsx-curly-brace-presence] Bail out in warning JSX expression when some chars exist #1458

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions docs/rules/jsx-curly-brace-presence.md
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ will be warned and fixed to:
<App prop="Hello world">Hello world</App>;
```

* If the rule is set to enforce curly braces and the strings have quotes, it will be fixed with double quotes for JSX children and the normal way for JSX attributes.
* If the rule is set to enforce curly braces and the strings have quotes, it will be fixed with double quotes for JSX children and the normal way for JSX attributes. Also, double quotes will be escaped in the fix.

For example:

Expand All @@ -137,12 +137,14 @@ will warned and fixed to:
<App prop={"Hello \"foo\" world"}>{"Hello 'foo' \"bar\" world"}</App>;
```

* If the rule is set to get rid of unnecessary curly braces and the strings have escaped characters, it will not warn or fix for JSX children because JSX expressions are necessary in this case. For instance:
* If the rule is set to get rid of unnecessary curly braces(JSX expression) and there are characters that need to be escaped in its JSX form, such as quote characters, [forbidden JSX text characters](https://facebook.github.io/jsx/), escaped characters and anything that looks like HTML entity names, the code will not be warned because the fix may make the code less readable.

The following pattern will not be given a warning even if `'never'` is passed.

```jsx
<Color text={"\u00a0"} />
<App>{"Hello \u00b7 world"}</App>;
<style type="text/css">{'.main { margin-top: 0; }'}</style>;
```

## When Not To Use It
Expand Down
82 changes: 61 additions & 21 deletions lib/rules/jsx-curly-brace-presence.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/**
* @fileoverview Enforce curly braces or disallow unnecessary curly brace in JSX
* @author Jacky Ho
* @author Simon Lydell
*/
'use strict';

Expand Down Expand Up @@ -59,18 +60,45 @@ module.exports = {
{props: ruleOptions, children: ruleOptions} :
Object.assign({}, DEFAULT_CONFIG, ruleOptions);

function containsBackslashForEscaping(rawStringValue) {
function containsLineTerminators(rawStringValue) {
return /[\n\r\u2028\u2029]/.test(rawStringValue);
}

function containsBackslash(rawStringValue) {
return rawStringValue.includes('\\');
}

function containsHTMLEntity(rawStringValue) {
return /&[A-Za-z\d#]+;/.test(rawStringValue);
}

function containsDisallowedJSXTextChars(rawStringValue) {
return /[{<>}]/.test(rawStringValue);
}

function containsQuoteCharacters(value) {
return /['"]/.test(value);
}

function escapeDoubleQuotes(rawStringValue) {
return rawStringValue.replace(/\\"/g, '"').replace(/"/g, '\\"');
}

function escapeBackslashes(rawStringValue) {
return rawStringValue.replace(/\\/g, '\\\\');
}

function needToEscapeCharacterForJSX(raw) {
return (
containsBackslash(raw) ||
containsHTMLEntity(raw) ||
containsDisallowedJSXTextChars(raw)
);
}

/**
* Report and fix an unnecessary curly brace violation on a node
* @param {ASTNode} node - The AST node with an unnecessary JSX expression
* @param {String} text - The text to replace the unnecessary JSX expression
*/
function reportUnnecessaryCurly(JSXExpressionNode) {
context.report({
Expand All @@ -83,11 +111,10 @@ module.exports = {

let textToReplace;
if (parentType === 'JSXAttribute') {
textToReplace = `"${escapeDoubleQuotes(
expressionType === 'TemplateLiteral' ?
expression.quasis[0].value.raw :
expression.raw.substring(1, expression.raw.length - 1)
)}"`;
textToReplace = `"${expressionType === 'TemplateLiteral' ?
expression.quasis[0].value.raw :
expression.raw.substring(1, expression.raw.length - 1)
}"`;
} else {
textToReplace = expressionType === 'TemplateLiteral' ?
expression.quasis[0].value.cooked : expression.value;
Expand All @@ -103,34 +130,51 @@ module.exports = {
node: literalNode,
message: 'Need to wrap this literal in a JSX expression.',
fix: function(fixer) {
// If a HTML entity name is found, bail out because it can be fixed
// by either using the real character or the unicode equivalent.
// If it contains any line terminator character, bail out as well.
if (
containsHTMLEntity(literalNode.raw) ||
containsLineTerminators(literalNode.raw)
) {
return null;
}

const expression = literalNode.parent.type === 'JSXAttribute' ?
`{"${escapeDoubleQuotes(
`{"${escapeDoubleQuotes(escapeBackslashes(
Copy link

Choose a reason for hiding this comment

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

We also need to escape line terminators here.

Copy link
Contributor Author

@jackyho112 jackyho112 Sep 30, 2017

Choose a reason for hiding this comment

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

I actually wanted to ask you about them. I know you mentioned them here.

similarly for the line terminators: CR, LF, U+2028 and U+2029

I am not quite familiar with those. Do you type exactly those characters of any of the sets into the string to terminate a line? I tried having them in a JSX expression and it just displays those characters. Sorry about my lack of knowledge on the subject matter. 😀

Copy link

@lydell lydell Oct 1, 2017

Choose a reason for hiding this comment

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

Press the Enter key to type a LF (\n) (or most likely CRLF \r\n on Windows).

You can also run one of the following in the Chrome or Firefox dev tools console to copy the character to the clipboard:

  • copy('\n')
  • copy('\r')
  • copy('\u2028')
  • copy('\u2029')

Then you can simply paste the character.

But since the tests are written as JavaScript strings you should be able to use JavaScript escapes:

input: '<div propWithLineTerminators="\n\r\u2028\u2029"/>',

Copy link
Contributor Author

@jackyho112 jackyho112 Oct 1, 2017

Choose a reason for hiding this comment

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

For the example you mentioned, the output would be <div propWithLineTerminators={"\\n\\r\\u2028\\u2029"}/>? Because these characters would be displayed as they are in a JSX attribute.

In that case, wouldn't the parser have already handled that?

For <div propWithLineTerminators="\n\r\u2028\u2029"/>

screen shot 2017-10-01 at 12 44 28 pm

Copy link

Choose a reason for hiding this comment

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

Actually, I think we should bail out if we find any line terminators at all, because of Babel's whitespace stripping thing.

literalNode.raw.substring(1, literalNode.raw.length - 1)
)}"}` :
))}"}` :
`{${JSON.stringify(literalNode.value)}}`;

return fixer.replaceText(literalNode, expression);
}
});
}

// Bail out if there is any character that needs to be escaped in JSX
// because escaping decreases readiblity and the original code may be more
// readible anyway or intentional for other specific reasons
function lintUnnecessaryCurly(JSXExpressionNode) {
const expression = JSXExpressionNode.expression;
const expressionType = expression.type;
const parentType = JSXExpressionNode.parent.type;

if (
expressionType === 'Literal' &&
typeof expression.value === 'string' && (
parentType === 'JSXAttribute' ||
!containsBackslashForEscaping(expression.raw))
typeof expression.value === 'string' &&
!needToEscapeCharacterForJSX(expression.raw) && (
parentType === 'JSXElement' ||
!containsQuoteCharacters(expression.value)
)
) {
reportUnnecessaryCurly(JSXExpressionNode);
} else if (
expressionType === 'TemplateLiteral' &&
expression.expressions.length === 0 && (
parentType === 'JSXAttribute' ||
!containsBackslashForEscaping(expression.quasis[0].value.raw))
expression.expressions.length === 0 &&
!needToEscapeCharacterForJSX(expression.quasis[0].value.raw) && (
parentType === 'JSXElement' ||
!containsQuoteCharacters(expression.quasis[0].value.cooked)
)
) {
reportUnnecessaryCurly(JSXExpressionNode);
}
Expand Down Expand Up @@ -170,17 +214,13 @@ module.exports = {

return {
JSXExpressionContainer: node => {
const parent = node.parent;

if (shouldCheckForUnnecessaryCurly(parent, userConfig)) {
if (shouldCheckForUnnecessaryCurly(node.parent, userConfig)) {
lintUnnecessaryCurly(node);
}
},

Literal: node => {
const parentType = node.parent.type;

if (shouldCheckForMissingCurly(parentType, userConfig)) {
if (shouldCheckForMissingCurly(node.parent.type, userConfig)) {
reportMissingCurly(node);
}
}
Expand Down
Loading