react/jsx-curly-brace-presence autofix doesn't escape string children

[blixt]

This is a small issue, but it can lead to lint errors from other rules (react/no-unescaped-entities):

<code>{`let x = "hello"`}</code>

becomes

<code>let x = "hello"</code>

which should be this instead:

<code>let x = &quot;hello&quot;</code>

[ljharb]

It’s always totally fine for an autofix to result in things that other lint rules can warn on or autofix - the CLI makes multiple passes which should clean it up.

There’s no way for one rule to know about other rules, or their configuration, so we always have to pick a default autofix behavior.

[blixt]

This one doesn't auto fix though, and since it happens as part of the recommended config of this very same repo, shouldn't an attempt be made to allow things to work "out of the box"? I also think there's little ambiguity here - the quotes should be escaped when moved from JS to JSX.

[ljharb]

In the context of a jsx element's children, there's no need to escape them, so there's no ambiguity.

The problem is that there's no rule for the reverse - so if somebody wants unescaped quotes, right now they'd just disable that one rule, but with your suggestion, they'd have to also disable, or manually fix, this rule.

[michaeljota]

What about having an option to require curly braces on strings with unescaped entities? This way this rule shouldn't about the other, but still could auto-fix all children. What do you think?

[ljharb]

I guess I'm not really clear on what you're asking for.

The reason no-unescaped-entities doesn't autofix is because it can't know the author's intention - I don't think there's any way to fix that.

[michaeljota]

I think you have a list of what is considered an "unescaped entity", so you could use the rule no-unescaped-entities. Right?

I understand that rules shouldn't know about other rules, and I agree with that.

But, instead of having the jsx-curly-brace-presence to know whether no-unescaped-entities is on or off, the rule could have an option to ignore the curly braces on string children.

Using OP example

<code>{`let x = "hello"`}</code>

Would stay the same with this option, because the rule will ignore this pattern, as it would identify this as a children prop, with unescaped entities within.

[ljharb]

Right, but the point of this rule is to disallow unnecessary curly braces entirely.

[michaeljota]

But in this case, the curly braces are necessary, because they are using to escape those entities.

[ljharb]

They're not strictly necessary - they're a convenience (with the no-unescaped-entities rule). You can always escape them.

[michaeljota]

But we would be using this to escape them. Wouldn't we?

[ljharb]

No - in no-unescaped-entities, the curly braces are used to inform the rule that the lack of escaping is intentional.

[aarowman]

Basically, it's an issue of convenience and readability. Yes, technically you don't NEED this to exist, but it would be a great addition.

For example,

Original goal:

<span>{`Click "Save" to continue`}</span>

is more readable to me than

<span>Click &quot;Save&quot; to continue</span>

and the final html is the same result.

The auto-fix here tries to change it from original to

<span>Click "Save" to continue</span>

which flags no-unescaped-entities, and is undesirable. Yes, we can fix it by using the encoded value instead, but the original code is best.

The best workaround I've used is to create it as a variable first:

const myText = `Click "Save" to continue`;

...

<span>{myText}</span>

It would be nice not to require this though.

Notes:

1. We could use either ` or ' for strings here.
   `string with " quotes` could be replaced with 'string with " quotes' and everything is still the same as described above.

2. It's the same with {"Don't stop believin'!"} (single ticks)

3. Strangely it isn't getting flagged if you have 2 curly brace sections in a row:

   <span>
       {'Click "Save"'}
       {' to continue'}
   </span>

   or if you put a comment inside it:

   <span>{'Click "Save" to continue' /* ignore me */}</span>

   Maybe this is a separate issue

[ljharb]

@aarowman since straight quotes are always typographically incorrect in prose, the MOST readable is also the only correct one:

<span>Click “Save” to continue</span>

[aarowman]

@ljharb

since straight quotes are always typographically incorrect in prose

Even if it's more 'typographically correct', I personally always dislike seeing “ and ” anywhere in life (this is due to utf8/encoding issues I've seen in the past). Web sites are also noted as an exception because it's not a written document like legal paperwork.

e.g. https://www.proofreadnow.com/blog/bid/42842/Question-Straight-or-Curved-Quotes

Either way, the code example above with let x = "hello" is still applicable. Or if using single ticks. Why does it matter what is preferred "in prose" ? The library should handle the various scenarios and use cases either way. I imagine many devs aren't putting the curly quotes on an average app.

[ntkoopman]

The documentation says it should keep the version with curly braces:

https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-curly-brace-presence.md#edge-cases

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

But it doesn't seem to work correctly.

[ljharb]

This seems pretty answered. Please file a new issue with repro code if there's something that still needs fixing.

[aarowman]

@ljharb

This seems pretty answered.

I disagree - even just looking at the last comment (by @ntkoopman) :

But it doesn't seem to work correctly

How has this been answered already if something is not working correctly?

@blixt do you think this issue is resolved? Or should we create a new issue / feature request?

[ljharb]

@aarowman someone claiming it doesn’t work but not providing repro code isn’t actionable. If there’s code that can reproduce the problem, I’ll immediately reopen this - but lacking that, my tests say it’s working properly.

[aarowman]

@ljharb Sure, that makes sense. You didn't ask for one though, just closed it.

@ntkoopman you said it doesn't work correctly as documented. Can you create an example that shows it? Or anyone else (@blixt this is your issue, or @michaeljota you mentioned it a lot too). My example is proprietary and can't be shared.

If everyone agrees that the workarounds are good enough, then we can leave it closed. It just seemed a little hasty to me to close it suddenly without checking back in first. The last comment was that something didn't work, then it got closed saying "this seems pretty answered". That seemed a little off.

[ljharb]

@aarowman the OP was answered. All the folks piling on instead of making a separate issue can file one :-)

[aarowman]

the OP was answered

I didn't realize this was the case. As I read it, the issue was not resolved, it was just replied to say it shouldn't behave that way (and possibly misleading from documentation) instead of actually resolving anything. Maybe I'm the only one who interpreted it that way - that's why I asked others to weigh in.

[ntkoopman]

I created #3214 for the issue I encountered