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 removes braces that are required #1449

Closed
modosc opened this issue Sep 26, 2017 · 4 comments · Fixed by #1458
Closed

jsx-curly-brace-presence removes braces that are required #1449

modosc opened this issue Sep 26, 2017 · 4 comments · Fixed by #1458

Comments

@modosc
Copy link

modosc commented Sep 26, 2017

<style type="text/css">{'.mt0 { margin-top: 0; }'}</style>

with the 7.4.0 release these braces are removed and then eslint chokes on the syntax.

@lydell
Copy link

lydell commented Sep 27, 2017

The spec lists all the characters that are disallowed:

JSXTextCharacter : SourceCharacter but not one of {, <, > or }

Note that the spec does not define any way escaping them, so the linter rule must either skip reporting and converting string literals with such characters in them to JSXText, or use the defacto standard of using HTML-style escapes supported by Babel: &#123;, &lt;, &gt;, &#125;, respectively. facebook/jsx#4

@lydell
Copy link

lydell commented Sep 28, 2017

Here's a problem I ran into in a production code base: <Color text={"\u00a0"}/> was auto-fixed into <Color text="\u00a0"/>, which changed the text prop from a non-breaking space to the literal string \u00a0.

TL;DR The jsx-curly-brace-presence must either:

  • Bail out if a problematic character is found.
  • Implement the below escaping logic.

Escaping logic

When going from JS to JSX, one has to:

  • Escape everything that looks like a JSX &-escape. For example, &nbsp;&amp;nbsp;. One can either replace all & characters with &amp;, or only valid &-escapes (not &foobar;, for example).
  • Replace all JS \-escapes with JSX &-escapes, or the literal unicode character. For example, \u00a0&nbsp; (or &#xa0;), \a (useless escape) → a, \ractual CR (or &#13;), \nspace! (Babel seems to do so at least).
  • Escape all disallowed characters in JSX, depending on context: "&quot;, or '&apos;, or {<>}&#123;&lt;&gt;&#125;.

When going from JSX to JS, one has to:

  • Escape everything that looks like a JS \-escape. For example, \n\\n.
  • Replace all JSX &-escapes with JS \-escapes. For example, &nbsp;\u00a0 (or \xa0).
  • Escape quotes and line terminator: "\" or '\', similarly for the line terminators: CR, LF, U+2028 and U+2029.

Note that I think it is important to for example convert \u00a0 into &nbsp; (or &#a0;), and not into   (an actual non-breaking space character), since the author probably chose to use an escape instead of the real character in the first place.

White space

White space handling is not part of the JSX spec: facebook/jsx#6. It is up to each implementation of JSX. I guess Babel is the defacto standard, but Facebook has some internal tooling with slightly different rules: prettier/prettier#2279

Either way check out the difference in white space in the output here:

<a a="start
      
      end"/>
;
<a a={"start\
      \
      end"}/>
"use strict";

React.createElement("a", { a: "start end" });
React.createElement("a", { a: "start\
      \
      end" });

I haven’t had the time to think about how this should be handled yet.

Notes

The babel repl is super handy for checking how Babel converts escapes.

Neither quoted attribute values in JSX nor JSXText have JS-style \-escapes. Compare the spec with string literals in the ES spec. (A SourceCharacter is "any Unicode code point".)

&-escapes are undocumented but I guess how Babel treats them is the defacto standard.

Many of the jsx-curly-brace-presence test are wrong (the fixed code output sometimes even contain syntax errors). For example, these ones: https://github.com/yannickcr/eslint-plugin-react/blob/1f14fad95d28672f1eb16dbe962557a7072d16e9/tests/lib/rules/jsx-curly-brace-presence.js#L183-L217

My opinion

I'm in favor of choosing the "bail out approach" because:

  • It is simpler.
  • <style type="text/css">{'.mt0 { margin-top: 0; }'}</style> is much more readable than <style type="text/css">.mt0 &#123; margin-top: 0; &#125;</style>.

I could be interested in implementing this when we reach a consensus and I find the time.

Even though I really like this rule (big thanks to @jackyho112), I’m going to turn it off for now since it is not safe to use :(

@jackyho112
Copy link
Contributor

jackyho112 commented Sep 28, 2017

@lydell

Thanks for the input! That was extremely helpful.

When I was implementing this, I referred mostly to the React docs. But the JSX specs are definitely better! I can’t believe I didn’t think of it at the time.

I am in favor of the bailout approach as well. I will try to get a PR up by this week to fix this.

@lydell
Copy link

lydell commented Sep 29, 2017

@jackyho112 Awesome! Ping me in your PR and I'll help reviewing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants