fix: Correctly generate StringLiteral in JSXAttribute

[liuxingbaoyu]

Q	A
Fixed Issues?	Fixes #15472, Fixes #16065
Patch: Bug Fix?	√
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	√
Documentation PR Link
Any Dependency Changes?
License	MIT

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/55798/

[nicolo-ribaudo]

Unfortunately we cannot do this in a Babel 7 release, but I think that a better fix would be to not handle HTML entities in the parser/generator (so the string's .value would still be &), and instead convert them in the JSX transform plugin.

[nicolo-ribaudo]

Is it safe to do just this, avoiding the extra dependency?

Suggested change

	const val = inJsx
	? `"${htmlEncode(node.value, {
	level: EntityLevel.HTML,
	mode: EncodingMode.UTF8,
	})}"`
	: jsesc(node.value, this.format.jsescOption);
	let { value } = node.value;
	if (inJsx) value = value.replace(/&/g, "&");
	value = jsesc(value, this.format.jsescOption);

[liuxingbaoyu]

I'm not sure if & is necessary, " definitely needs this.
Also for \, it seems that jsx should not escape.

[andersk]

Right.

• " must be escaped so we can round-trip " → """ → ".
• & must be escaped so we can round-trip """ → """"" → """.
• \ must not be escaped as \\, because a JSX parser won’t unescape that way.

[liuxingbaoyu]

Unfortunately we cannot do this in a Babel 7 release

This shouldn't be a breaking change, is there something I haven't noticed?

but I think that a better fix would be to not handle HTML entities in the parser/generator (so the string's .value would still be &), and instead convert them in the JSX transform plugin.

I'm actually leaning toward creating a new node type in the same form as it does now. But this will affect a lot in compatibility, so I chose to set .extra,

[liuxingbaoyu]

Now will no longer affect the bundled size.

[andersk]

escapeUTF8 escapes ' as ', which is an HTML 5 addition that is not allowed by JSX.

I think escapeAttribute is the right function?

[liuxingbaoyu]

Thanks!

[liuxingbaoyu]

This didn't contain <>, nothing seemed to fit, so I copy-pasted the code.

[andersk]

Inside a double-quoted attribute, escaping <> doesn’t hurt but isn’t strictly necessary. The parser is only looking for entity references and a terminating ".

[liuxingbaoyu]

#16065
This has the same reason. StringLiteral in js will not become two lines, which is not the case in jsx.

[liuxingbaoyu]

@nicolo-ribaudo Can you re-review this? :)
Thanks!

[JLHwung]

CI error seems related.

[nicolo-ribaudo]

Could we not handle HTML entities here & while parsing at all, and instead handle them in the JSX transform? 🤔

i.e. <X y="a &lt; b" /> is just a normal string according to the parser/generator, and when transforming it to plain JS the plugin will unescape HTML entities and other JSX weirdnesses. By doing so, we need to handle JSX encoding just in one place (the plugin) rather than twice (the parser and the generator).

[andersk]

No. A string literal must be escaped differently for the different contexts. In JS, \ must be escaped as \\, and in JSX, it must not. In JS, " must be escaped as \", and in JSX, it must be escaped as ". These differences must be handled by the parser and generator to support users of the parser and generator that transplant a string literal node from one context to another. And this doesn’t happen in just one plugin; for example, the React plugin transforms JSX differently from the Solid plugin. See #15472.

[liuxingbaoyu]

That would be a huge breaking change, meaning escaping would have to be handled manually by plugins and users. Given that it's not difficult for us to support this, and we've supported it in the past, there are some users downstream who need it, so I'd be inclined to leave it as is.
And even so, we still need to check whether a StringLiteral is in jsx or js. :)