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

Newline having a trailing whitespace character is removed in JSX attribute value #10356

Open
beliayeu opened this issue Aug 20, 2019 · 6 comments · May be fixed by #10359

Comments

@beliayeu
Copy link

commented Aug 20, 2019

Bug Report

Current Behavior
new line (\n character) is removed from jsx attribute value when there is a whitespace character (\s+) right after new line.
new line (\n character) is NOT removed from jsx attribute value when there are no whitespace characters (\s+) right after new line.

Input Code

Expected behavior/code
Newline should be processed more consistently. Either it should be removed or not removed in both cases mentioned in the current behavior.

Impact
react helmet is used for injecting scripts to the document head. react helmet allows to add inline scripts using innerHTML attribute. once we add inline scripts containing inline comments then we end up with broken script produced by react render

example

<Helmet>
    <script innerHTML="
var firstVariable = 1; //test

var lostVariable = 2;
" />
</Helmet>

will produce

<script data-react-helmet="true" >
var firstVariable = 1; //test var lostVariable = 2;
</script>

Possible Solution
Option 1. do not remove new line, remove only trailing whitespace characters.

change

value.value = value.value.replace(/\n\s+/g, " ");

into

value.value = value.value.replace(/\n\s+/g, "\n");

in babel-helper-builder-react-jsx

Concerns
were there any reasons why the new line was replaced with whitespace?

@babel-bot

This comment has been minimized.

Copy link
Collaborator

commented Aug 20, 2019

Hey @beliayeu! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack community that typically always has someone willing to help. You can sign-up here for an invite.

@JLHwung

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

I don't think we should strip \n\s+ from value.vlaue at all. The JSX Specification does not forbid JSXDoubleStringCharacter contains \n . Besides, the JSX parser acorn-jsx does not exhibit this behavior.

@beliayeu Could you open up a PR to remove value.value transforms?

@loganfsmyth

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

Besides, the JSX parser acorn-jsx does not exhibit this behavior.

This is a feature of JSX transformation, not of the parser, so it isn't something that acorn-jsx would be expected to handle.

I'm sure we could debate about this, but as-is, this is a feature React expects, and changing this would absolutely break anyone relying on the existing whitespace behavior. If you look at the react docs, it states;

JSX removes whitespace at the beginning and ending of a line. It also removes blank lines. New lines adjacent to tags are removed; new lines that occur in the middle of string literals are condensed into a single space.

If whitespace is important for your usecase, you should use a construct that is guaranteed to preserve whitespace, so for instance

<script innerHTML={`
var firstVariable = 1; //test

var lostVariable = 2;
`} />
@beliayeu

This comment has been minimized.

Copy link
Author

commented Aug 21, 2019

JSX removes whitespace at the beginning and ending of a line. It also removes blank lines. New lines adjacent to tags are removed; new lines that occur in the middle of string literals are condensed into a single space.

that is true for new lines adjacent to tags

<e>   
</e>

results in

React.createElement("e", null);

but as far as I can see that is not always true for string literals

<e a="foo
bar"/>

results in (new line is not condensed into a space)

React.createElement("e", {
  a: "foo\nbar"
});

but new line with trailing space is condensed into a single space

<e a="foo
 bar"/>

results in

React.createElement("e", {
  a: "foo bar"
});

If whitespace is important for your usecase, you should use a construct that is guaranteed to preserve whitespace, so for instance

the issue I have with it is unexpected decoding of characters

<Helmet>
    <script innerHTML="var v1 = &quot;\x3ch1&quot;;" />
    <script>{`var v2 = "\x3ch1";`}</script>
 </Helmet>

results in

<script data-react-helmet="true" >var v1 = "\x3ch1";</script>
<script data-react-helmet="true" >var v2 = "<h1";</script>

\x3c is decoded into < in the second script

but I need to perform additional root cause analysis to understand the problem better, most likely the problem is not related to babel project

@JLHwung

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

JSX removes whitespace at the beginning and ending of a line. It also removes blank lines. New lines adjacent to tags are removed; new lines that occur in the middle of string literals are condensed into a single space.

The behavior above is defined in the context of JSX children, to be consistent with HTML text node. But how whitespace in JSXAttributeValue is treated, is not clearly defined. Note that HTML does not require whitespace in quoted attribute value to be transformed (playground), the JSXAttributeValue is expected to align with HTML attribute value.

I'm sure we could debate about this, but as-is, this is a feature React expects, and changing this would absolutely break anyone relying on the existing whitespace behavior.

The current behavior may date back to acornjs/acorn-jsx#16 (comment). Given the behavior has been there for 4 years, it is, unfortunately, de facto.

Unless the JSX clearly defined how JSXAttributeValue should be processed, and given that there is workaround, we may leave it as compromise.

@loganfsmyth

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

So we're on the same page, I 100% agree that this is all poorly defined. The best we have is defacto-standard behavior. I'd be curious to see what Typescript does in this case as well.

If someone wants to explore what changing this might look like, that seems like a great idea. I do think any changes to this would need to be backed by a specific goal of making the behavior more consistent across implementations, and it would need to be behind a flag unless until a major release. It'd also expect explicit feedback from the React team and other libraries that rely on JSX.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.