JSX whitespace coalescing rules

[syranide]

Current rules

{1}··Aaa··Bbb··{2}··{3}  →  {1}·Aaa··Bbb·{2}{3}
{1}¬¬Aaa¬¬Bbb¬¬{2}¬¬{3}  →  {1}·Aaa·Bbb·{2}{3}

While they seem reasonable to some extent, they are quite nonsensical from a theoretical perspective.

{expr} is treated identically to <tag> and </tag>
· space ¬ newline

Whitespace rules

1. Remove whitespace, makes no sense whatsoever.

2. Replace whitespace with a space, consistent output
Minimal whitespace is always produced and it is consistent with normal browser rendering, any and all special whitespace requirements are explicit and easy to spot in the code.

3. Replace whitespace with spaces, faithful to source code
Unintended double-spaces are possible but unlikely and normally renders identically to 2. Interesting in that it might encourage less hacky   and more white-space: pre, all things considered it seems like a feature.

4. Keep whitespace as is, identical to source
Normally renders identically to 3 but relying on non-escaped non-space whitespace seems really finicky and hardly something that should be encouraged, non-space whitespace should be explicitly injected.

1.  {1}··Aaa··Bbb··{2}··{3}  →  {1}AaaBbb{2}{3}
2.  {1}··Aaa··Bbb··{2}··{3}  →  {1}·Aaa·Bbb·{2}·{3}
3.  {1}··Aaa··Bbb··{2}··{3}  →  {1}··Aaa··Bbb··{2}··{3}
4.  {1}··Aaa··Bbb··{2}··{3}  →  {1}··Aaa··Bbb··{2}··{3}

Newline rules

A. Remove newlines, consistent and uncomplicated
Text requires explicit whitespace injection, slightly cumbersome and ugly in "worst case".

B. Remove newlines but insert a space between text
Most likely what the user wants without having to be overly explicit, implicit space can be overridden with braces.

C. Remove newlines, but insert a space before/after text
This may seem kind of interesting at first, even though it's a bit weird. However this is, in its basic form, the root cause of all the excess whitespace that is creeping in at the start and end of every text with the current rules. In my opinion, the only way this rule makes sense (without being a burden) is if it does not match after an opening tag or before a closing tag... simply put, it would only add a space when text is sibling to a node (or a brace). Which would make it quite magic, which isn't necessarily bad, but my gut feeling says it is. Being explicit in ambiguous cases is preferable to it sometimes making the wrong decision (especially when it comes to tests), and the rules would be very complex to explain.

D. Remove newlines but insert a newline between text
Likely unintended and very unlikely to be useful, easy to break if relying on identical text output, implicit newline can be overriden with braces.

E. Remove newlines before and after tags, faithful to source code
Although there are vaguely potential uses for this, that much text should never be inlined, be explicit if you absolutely must, implicit newlines can be overriden with braces.

F. Keep newlines as is, identical to source but whitespace is trimmed
This is a horrible feature in HTML, let's not go there.

A.  {1}¬¬Aaa¬¬Bbb¬¬{2}¬¬{3}  →  {1}AaaBbb{2}{3}
B.  {1}¬¬Aaa¬¬Bbb¬¬{2}¬¬{3}  →  {1}Aaa·Bbb{2}{3}
C.  {1}¬¬Aaa¬¬Bbb¬¬{2}¬¬{3}  →  {1}·Aaa·Bbb·{2}{3}
D.  {1}¬¬Aaa¬¬Bbb¬¬{2}¬¬{3}  →  {1}Aaa¬Bbb{2}{3}
E.  {1}¬¬Aaa¬¬Bbb¬¬{2}¬¬{3}  →  {1}Aaa¬¬Bbb{2}{3}
F.  {1}¬¬Aaa¬¬Bbb¬¬{2}¬¬{3}  →  {1}¬¬Aaa¬¬Bbb¬¬{2}¬¬{3}

All lines are trimmed from leading and trailing whitespace, indenting code must never interfere with output in my opinion.

---

Contenders

2A/2B. Minimal whitespace, consistent output, encourage explicit whitespace
The user is encouraged to be explicit in special-cases. With 2A we assume nothing, slightly cumbersome/ugly when dealing with moderate amounts of text, but very straight-forward. With 2B we reasonably assume that multi line texts should be separated by a space, the user can override if necessary.

2.  {1}··Aaa··Bbb··{2}··{3}  →  {1}·Aaa·Bbb·{2}·{3}
A.  {1}¬¬Aaa¬¬Bbb¬¬{2}¬¬{3}  →  {1}AaaBbb{2}{3}
B.  {1}¬¬Aaa¬¬Bbb¬¬{2}¬¬{3}  →  {1}Aaa·Bbb{2}{3}

3A/B. Additional whitespace is most likely intentional, but very rarely useful
Although in my opinion this is largely because of the largely unknown white-space: pre, although still limited in usefulness. Otherwise same as 2A/B, except that 3B has a bit weird mix of keeping and generating whitespace, but results are intuitive and straight-forward still.

3.  {1}··Aaa··Bbb··{2}··{3}  →  {1}··Aaa··Bbb··{2}··{3}
A.  {1}¬¬Aaa¬¬Bbb¬¬{2}¬¬{3}  →  {1}AaaBbb{2}{3}
B.  {1}¬¬Aaa¬¬Bbb¬¬{2}¬¬{3}  →  {1}Aaa·Bbb{2}{3}

My personal vote goes to 3B at this time.

It's also worth mentioning that ALL whitespace rules can be overriden with braces, so none of these rules are able to exclude any useful behavior, it can only make the resulting code more or less ugly.

I would also strongly recommend #489 to cut down on braces exploding into spans (especially when transforming existing code as it would add braces in a lot of places, generating lots and lots of unnecessary spans).

---

Legacy JSX

Seeing as there's probably already a bit of code out there using JSX, I've written a first implementation of a tool that converts existing JSX files so that they continue to produce identical output even after switching to these new whitespace rules (final implementation is awaiting final verdict on which whitespace rules we want). This obviously potentially uglifies the code a bit with {' '}, but they are easy to spot, and each transformation in the tool is optional, including even using {(' ')} as space to make it more obvious what's auto-generated.

https://github.com/syranide/react/compare/jsx-legacy-whitespace-tool

[syranide]

@jeffmo Sanity check function renderXJSLiteral(object, isLast, state, start, end), start and end seems to never be used, should I remove them along with the code that uses them inside that function? Or am I missing something.

[jeffmo]

I'm finding it a little difficult to follow what's going on in here, but I think this is the source of failure when transforming something like:

/**
 * @jsx React.DOM
 */
<img alt="" src="" />

which, after these changes, now transforms to

/**
 * @jsx React.DOM
 */
React.DOM.img( {alt:"" src:""} )

In that scenario, line here is "" -- and thus falsey.

On a side note (for future ref): I believe our style guide asks that you always use curly braces and to put conditional bodies on a new line

[jeffmo]

on renderXJSLiteral -- we use it for another transform that we have in-house, so we'll need to leave it for now

[syranide]

@jeffmo
Rebased so it properly uses utils.*
Fixed some minor style issues in #489
I compared the output of old JSX with that of new JSX after refactoring on a large test-file I have, it produced identical whitespace
I also separated https://github.com/syranide/react/compare/jsx-legacy-whitespace-tool into its own tool, so it no longer replaces the real JSXTransformer. So I added bin/jsx-refactor and vendor/jsxrefactor/, feel free to call them whatever you want (I honestly don't know what to call them)? (If you want to "ship it" with React)

More thoroughly documented the refactoring tool, but I'm not sure how useful/understandable it really is. I doubt anyone would be able to make an informed decision on which transformations to disable (I would keep all of them enabled). In my opinion the annotated space {'\x20'} is also the best choice for the spaces we insert, especially if my jsxconcat PR is merged so that it is recognized as a fixed string and thus concatenated.

It's also worth pointing out that running jsx-refactor multiple times on the same source has no effect, due to the way the rules work. So it's a safe operation.

From my perspective I feel good about the current code and as far as I can tell, it follows your code style. Obviously, feel free to object :) and like I said on IRC, I'll be available until friday, but after that I won't be back until next sunday.

Although it is not required I do recommend considering #489 as well, without it the explicit whitespace injected by the refactoring tool will cause quite a lot of additional DOM spans to be rendered, as well as when explicitly inserting whitespace as you're aware can be quite common.

[jeffmo]

It looks like

<div>
  stuff
</div>

transforms to

React.DOM.div(null,
  " stuff "
)

Note the spaces around "stuff". Don't the rules that we've settled on suggest that there shouldn't be spaces here? (At least thats what the conversion tool's output suggests...)

[syranide]

@jeffmo Correct, and it produces the correct output (without spaces) for me.

React.DOM.div(null,
  "stuff"
)

Is it perhaps a repeat of yesterday?

[22:29] lbljeffmo: syranide: ok I'm a clown -- I was accidentally using the old transformer when I though I was using yours

Hehe

(Just to be on the safe-side, note that JSXTranformer in my refactoring tool branch does NOT have the whitespace patch applied, they are separate branches)

[vjeux]

What's the status here?

[jeffmo]

I think we need a solution to span-wrapping of {' '} (since the codemod script puts that in place to stub-out where whitespace would have been before...but is no longer after this diff).

Adding spans can break styling and externally-expected DOM structures -- so we need some kind of workaround for the transition (or maybe we bite the bullet). @syranide: Were you planning to talk to @petehunt and @sebmarkbage about fixing the spans issue at a deeper level?

[jeffmo]

(other than that, I think this is good to go)

[syranide]

@jeffmo #489 solves that specific case, that PR previously made sense by itself as {' '} was a common occurence. Now, it serves to prevent the whitespace inserted by the "codemod" script from creating spans. It's also still generally useful (but not as useful) as it concatenates all fixed strings (so basically, wherever you explicitly insert any whitespace). It's simply an optimization of the JSX output and should have no practical disadvantages (other than possibly "philosophical", and code complexity). The only possible difference it can have is that fewer spans may be generated and possibly that no spans at all will be generated in certain cases (as a string appearing as the only child creates no spans).

However, it seems to me like React generating spans is simply the way it is currently. In my opinion, in this context, using spans without a className is asking for trouble, and assuming that React does create spans is pure stupidity. So as #489 has the possibility of reducing the amount of generated spans and thus altering the DOM structure, anyone applying any common sense for React should not be at all affected.

That being said, I did look into it. There are basically two possible solutions that I considered:

1. Use a fragment for innerHTML instead of doing it directly on the DOM. Then, traverse all the nodes (this can be solved using getElementById if absolute performance is necessary), replacing any implicitly generated spans with the TextNode inside and transfer the reactid from the span to the TextNode. Finally simply insert the fragment into the DOM. However, there is no way to attach a reactid to a TextNode other than adding a custom property to it (which may not be optimal, but it works), however this seemingly cannot be solved at all in IE8 using the reactid-approach (you can't add a property or defineProperty on TextNodes in IE8).
2. (I didn't really research this to any greater extent) Do not generate any implicitly generated spans and insert into the DOM as usual. When re-rendering, treat any consecutive TextNodes as being concatenated and refer to them as the "previousSibling of this DOM-node". This would be the cleanest/fastest approach, but it appears to me (and I'm definitely no authority on this) that this would require significant effort to solve currently, and if TextNodes do split it automatically (certain not so old versions of FF do this) would break Reacts _mountIndex used for reordering elements (and possibly would regardless because of the concatenation).
3. (Not really a solution) Another thing to consider is to use a custom element in-place of the implicitly generated spans, such as reacttext, which would prevent any non-* CSS-rules from interfering at least. The HTML-standard discourages custom elements (which is sensible), but they are supported by all browsers to my knowledge.

So, given the current state of React of reactid (which will be changing in the near future it seems), it currently seems to me like there exist no tractable solution for solving the span-issue, unless someone finds a workaround for the IE8-issue.

So #489 should suffice for any "issues" introduced by JSX and the "codemod", and should bring parity and possibly even improvements to any projects as compared to before this PR is introduced. That is, nothing can get worse, it may only change for the better in certain cases (and if you rely on such specific behavior, you're going to be in trouble anyway sooner or later).

[syranide]

@jeffmo Not saying we should do it, but one idea is to keep a legacy version of JSX(Transform) in the repository for the time being. So that anyone who feel they can't switch to the newer right away can stay behind a short while.

Also, what is your thoughts for the "codemod" tool? Bundle it with React for now? Point people towards my repository? (Just checking, I honestly have no preference)

[syranide]

The simplified (technical) explanation for the current whitespace rules would be:

All lines having leading or trailing whitespace are trimmed, all newlines are 
removed, adjacent text separated by newlines become separated by a single 
space. Any whitespace tabs are replaced with spaces. Strings inside 
expressions are unaffected.

[jeffmo]

This is a great summary, thanks.

one idea is to keep a legacy version of JSX(Transform) in the repository for the time being

That's not a bad idea -- but I'd be a little worried about maintenance and having to support the old + the new transform. I say we just bite the bullet and move forward

Also, what is your thoughts for the "codemod" tool?

I have a little CLI npm module I've packaged up (not published or checked in anywhere yet) that I've been using to codemod whole directories. We can probably just publish that and reference it in the CHANGELOG (as well as in the announcements for the next major rev cut)

Anyway, you've convinced me it's worth moving forward despite the spans problem, so let's land this damn thing already!

[syranide]

@jeffmo Sounds great!

[syranide]

@jeffmo PS. I have not yet thoroughly vetted my jsxconcat PR, so I may need to look it over once more if you're considering it.

[syranide]

@jeffmo Rebased (only a test had a non-conflict conflict).

[thesoftwarephilosopher]

What's the current status of how React transforms JSX whitespace? I'm trying to figure it out for vanillajsx.