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

jackyho112 · 2017-09-30T19:35:17Z

This is an attempt to fix #1449, so that the jsx-curly-brace-presence rule, when set to get rid of unnecessary curly braces(JSX expression), will bail out or deem a JSX expression necessary when a character that needs to be escaped in its JSX form is found in the expression. It also improves the fix in enforcing curly braces by escaping backslashes in its JSX form.

Gigantic thanks to @lydell for suggesting the implementation plan for this PR. I hope I didn't screw it up too much. 😄

Let me know if there is any improvement you have in mind.

Fixes #1479. Fixes #1449.

…ons when some chars exist

jackyho112 · 2017-09-30T19:51:30Z

lib/rules/jsx-curly-brace-presence.js

@@ -103,34 +127,42 @@ module.exports = {
        node: literalNode,
        message: 'Need to wrap this literal in a JSX expression.',
        fix: function(fixer) {
+          // Leave it to the author to fix as it can be fixed by either a
+          // real character or unicode
+          if (containsHTMLEntity(literalNode.raw)) {


@lydell

We could use String.fromCharCode here to fix but based on your comment, it may not be what the author has in mind. I think maybe it's best to keep it simple and let the author decide.

jackyho112 · 2017-09-30T19:56:17Z

lib/rules/jsx-curly-brace-presence.js

+      return rawStringValue.replace(/\\/g, '\\\\');
+    }
+
+    function needToEscapeCharacterForJSX(raw, cooked) {


Not sure if it is also necessary to check for patterns like U+2029 because it doesn't seem like it is recognized in both JS and JSX. Correct me if I am wrong.

lydell

Would be nice to see some whitespace tests as well. babel repl

lydell · 2017-09-30T19:57:00Z

lib/rules/jsx-curly-brace-presence.js

      return rawStringValue.includes('\\');
    }

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


There can also be a # after the &: {.

You could also use /regex/.text(value) to return a boolean instead of null or an array of matches. No need for the g modifer.

lydell · 2017-09-30T19:57:38Z

lib/rules/jsx-curly-brace-presence.js

+    }
+
+    function containsDisallowedJSXTextChars(rawStringValue) {
+      return rawStringValue.match(/{|<|>|}/g);


This can be written as /[{<>}]/.test(rawStringValue)

lydell · 2017-09-30T19:58:03Z

lib/rules/jsx-curly-brace-presence.js

+    }
+
+    function containsQuoteCharacters(value) {
+      return value.match(/'|"/g);


This can be written as /['"]/.test(value)

lydell · 2017-09-30T20:05:44Z

tests/lib/rules/jsx-curly-brace-presence.js

-      options: [{props: 'never'}],
-      errors: [{message: unnecessaryCurlyMessage}]
-    },
-    {
      code: '<MyComponent prop=\'bar\'>foo</MyComponent>',
      output: '<MyComponent prop={\"bar\"}>foo</MyComponent>',


\" is a useless escape here. Do you mean " or \\"? There are several other occurrences of this throughout this file; you might want to go through it from top to bottom correcting all the useless escapes you come across.

(It might even be worth using template literals for all/most of these tests to make them easier to read: No need to escape 's.)

I remember I did this because of an Eslint rule. But I agree with you. I will try to use template literals on at least the tests in which I have to escape quote characters.

IMO, it's fine to add /* eslint-disable that-rule */ at the top of this file if needed. You should be allowed to break rules when you can motivate why. But I'm not a maintainer of this repo so I can't say for sure if that's OK. But it would be sad if it's not allowed to make it easier to succeed writing correct tests.

lydell · 2017-09-30T20:13:15Z

tests/lib/rules/jsx-curly-brace-presence.js

@@ -338,30 +336,6 @@ ruleTester.run('jsx-curly-brace-presence', rule, {
      ]
    },
    {
-      code: '<App>{\'foo "bar"\'}</App>',
-      output: '<App>foo \"bar\"</App>',


This autofix could have been kept. <App>foo "bar"</App> is valid.

I don't remember – have we addressed or talked about this one?

@lydell

Yeah, since it is valid, I added back the fix for cases like this and the tests.

lydell · 2017-09-30T20:13:45Z

tests/lib/rules/jsx-curly-brace-presence.js

-    },
-    {
-      code: '<App>{\"foo \'bar\'\"}</App>',
-      output: '<App>foo \'bar\'</App>',


This autofix could also have been kept.

lydell · 2017-09-30T20:16:04Z

tests/lib/rules/jsx-curly-brace-presence.js

+    },
+    {
+      code: '<App prop=\'foo &middot; bar\' />',
+      output: '<App prop=\'foo &middot; bar\' />',


I can't spot the difference between code and output here.

lydell · 2017-09-30T20:16:19Z

tests/lib/rules/jsx-curly-brace-presence.js

+    },
+    {
+      code: '<App>foo &middot; bar</App>',
+      output: '<App>foo &middot; bar</App>',


Not here either.

lydell · 2017-09-30T20:32:56Z

lib/rules/jsx-curly-brace-presence.js

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


We also need to escape line terminators here.

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

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"/>',

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"/>

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

jackyho112 · 2017-09-30T22:20:14Z

@lydell

Can't thank you enough for the feedback! Will update the PR soon based on your suggestions!

lydell · 2017-10-01T19:18:00Z

tests/lib/rules/jsx-curly-brace-presence.js

+      errors: [
+        {message: missingCurlyMessage}, {message: missingCurlyMessage}
+      ],
+      options: ['always']


Check out the babel repl. It strips some whitespace. We either have to replicate that, or bail out.

Thanks for pointing it out! If we were to bail out, I would check for multiple whitespaces?

I think we should bail out if there's a line terminator in there. Multiple spaces on a single line should be fine.

jackyho112 · 2017-10-01T22:10:51Z

@lydell

I think I have covered most of the things you said. Let me know if you have any other improvement in mind. 😀

lydell · 2017-10-02T05:39:57Z

lib/rules/jsx-curly-brace-presence.js

@@ -59,6 +59,10 @@ module.exports = {
      {props: ruleOptions, children: ruleOptions} :
      Object.assign({}, DEFAULT_CONFIG, ruleOptions);

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


There's no need for the + here.

lydell

Awesome work on this, @jackyho112!

I can't find anything more to say on this on this PR, and can't think about any more edge cases to cover now.

(Note to other reviewers: All comments above are resolved. GitHub just simply can't know to mark them as outdated.)

lydell · 2017-10-04T18:05:55Z

lib/rules/jsx-curly-brace-presence.js

      return rawStringValue.includes('\\');
    }

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


Final little nitpick: The parentheses in the regex are unnecessary. (Sorry, I can't help myself when I see a regex.)

It's all good. I am actually quite grateful for all your regex comments. I learned a lot from them! 😄

ljharb

This LGTM but let's get lots of reviews on it.

lydell · 2017-10-10T06:09:22Z

Reviewers: Don't feel intimidated by the walls of comments me and @jackyho112 have exchanged! If you look a the implementation (one file), you'll see that it's pretty clear and concise. The rest of the changes are just docs and tests updates.

TL;DR on the whole issue: Some characters make it unclear* if braces are unnecessary or not. If we find such characters, bail out.

* Because readability can suffer badly.

jackyho112 added 2 commits September 30, 2017 12:31

Make jsx-curly-brace-presence bail out in getting rid of JSX expressi…

ad9aebe

…ons when some chars exist

Fix a typo in docs

2bd2d07

jackyho112 changed the title ~~Make jsx-curly-brace-presence bail out in warning JSX expression in some cases~~ [jsx-curly-brace-presence] Bail out in warning JSX expression when some characters exist Sep 30, 2017

jackyho112 commented Sep 30, 2017

View reviewed changes

lydell reviewed Sep 30, 2017

View reviewed changes

jackyho112 added 2 commits October 1, 2017 11:44

Take into account feedback

af2fe6a

Add whitespace tests

dc523b1

lydell reviewed Oct 1, 2017

View reviewed changes

Bail out for line-breakers in JSX

8751e92

jackyho112 changed the title ~~[jsx-curly-brace-presence] Bail out in warning JSX expression when some characters exist~~ [jsx-curly-brace-presence] Bail out in warning JSX expression when some chars exist Oct 1, 2017

lydell reviewed Oct 2, 2017

View reviewed changes

jackyho112 added 2 commits October 1, 2017 23:00

Get rid of unnecessary regex

c93dab8

Add an author

238140c

lydell approved these changes Oct 4, 2017

View reviewed changes

Improve a regex expression

c4ff64c

ljharb approved these changes Oct 5, 2017

View reviewed changes

ljharb requested review from yannickcr, lencioni and EvHaus October 5, 2017 05:37

ljharb merged commit 6e54f69 into jsx-eslint:master Oct 17, 2017

jackyho112 deleted the make-jsx-curly-brace-presence-not-care-about-some-cases branch October 17, 2017 05:39

lydell mentioned this pull request Nov 21, 2017

Braces around string literal in JSX props can be removed prettier/prettier#3303

Closed

ljharb mentioned this pull request Dec 10, 2017

React/JSX-curly-brace-presence is too strict airbnb/javascript#1656

Closed

lydell mentioned this pull request Jul 6, 2018

Add an option to print single quotes in JSX prettier/prettier#4798

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jackyho112 commented Sep 30, 2017 •

edited by ljharb

jackyho112 Sep 30, 2017 •

edited

jackyho112 Sep 30, 2017

lydell left a comment

lydell Sep 30, 2017

lydell Sep 30, 2017

lydell Sep 30, 2017

lydell Sep 30, 2017

jackyho112 Sep 30, 2017

lydell Oct 1, 2017

lydell Sep 30, 2017

lydell Oct 3, 2017

jackyho112 Oct 3, 2017 •

edited

lydell Sep 30, 2017

lydell Sep 30, 2017

lydell Sep 30, 2017

lydell Sep 30, 2017

jackyho112 Sep 30, 2017 •

edited

lydell Oct 1, 2017 •

edited

jackyho112 Oct 1, 2017 •

edited

lydell Oct 1, 2017

jackyho112 commented Sep 30, 2017

lydell Oct 1, 2017

jackyho112 Oct 1, 2017

lydell Oct 1, 2017

jackyho112 commented Oct 1, 2017

lydell Oct 2, 2017

lydell left a comment •

edited

lydell Oct 4, 2017

jackyho112 Oct 4, 2017

ljharb left a comment

lydell commented Oct 10, 2017

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

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

Conversation

jackyho112 commented Sep 30, 2017 • edited by ljharb

jackyho112 Sep 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lydell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackyho112 Oct 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackyho112 Sep 30, 2017 • edited

Choose a reason for hiding this comment

lydell Oct 1, 2017 • edited

Choose a reason for hiding this comment

jackyho112 Oct 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackyho112 commented Sep 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackyho112 commented Oct 1, 2017

Choose a reason for hiding this comment

lydell left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

lydell commented Oct 10, 2017

jackyho112 commented Sep 30, 2017 •

edited by ljharb

jackyho112 Sep 30, 2017 •

edited

jackyho112 Oct 3, 2017 •

edited

jackyho112 Sep 30, 2017 •

edited

lydell Oct 1, 2017 •

edited

jackyho112 Oct 1, 2017 •

edited

lydell left a comment •

edited