CR and whitespaces break rendering

[saendu]

Hi Troy

In your readme you refer using the jsx prop as follows:

jsx={`
      <h1>Header</h1>
      <InjectableComponent eventHandler={myEventHandler} />
    `}

The carriage return as well as other whitespaces between every <tag> (including components) however will break your code and nothing will be rendered.

I suggest removing all whitespaces between >< with a simple regex. E.g.:
.replace(/\>(\s|\n|\r|\t)*?\</g,"><")

If you want I can provide a pull request.

-Sandro

[TroyAlford]

What kind of breakage are you seeing? Could you please provide an example? There are a plethora of unit tests that utilize line breaks and other types of whitespace and show appropriate behavior coming out - so I'm a little surprised to hear you're having trouble here.

The component actually used to strip all whitespace in a manner similar to your suggestion - but simply doing that actually breaks things like <code> and <pre> tags. So if there is an issue, we need a more comprehensive solution than just stripping the spaces.

[saendu]

I think we are not talking about the same. I am not referring the whitespaces between <tag> </tag> I am talking about the whitespaces and CRs when using template-syntax.

Check this for a live demo https://codesandbox.io/s/k5pkklv6ov?module=%2Fcomponents%2FApp.js

[TroyAlford]

After playing with your demo quite a bit, I noticed that you're dependent upon React 15, rather than React 16. This won't work - because react-jsx-parser uses React.Fragment, which was introduced in React 16. If you update to 16.3.2 (latest at the time of this response) the example works as-is.

Without it, you will also find that jsx={'<h1>Foo</h1>'} won't work - because the naked string inside the tag is rendered inside a Fragment by the component.

[saendu]

Ok I understand. Unfortunatelly we are forced to stick with React 15 atm.
The new Fragments syntax came with version 16.2 as I am informed. Is there any intention supporting this with React pre-16.2 versions or even pre-16.0? E.g. stick with divs or no fragments (I know this is not perfect) and only using the new fragment syntax above 16.0/.2. I am not sure however if this would reintroduce the whitespace issues discussed in #31 #33 #44.

[TroyAlford]

I don't currently have any plans to support an older version, unfortunately. As you correctly linked, the Fragment method helped fix several outstanding issues that were more difficult to solve in React 15.

I would be open to having a secondary release, pinned to that version, if you guys (or others) were willing to help maintain it and help to keep feature parity. I'd also be open to refactoring the current version, if possible, to remove the dependency on Fragment - because then we probably get React 15 compatibility by default.

We would need to figure out how to continue correctly supporting whitespace parsing and wrapperless rendering. I'm open to ideas here, and would be happy to participate in a PR.

[saendu]

Hi Troy

Using Fragment makes absolutely sense from a 16 perspective, however making all 16.2> versions unsupported. I do not understand the whitespace parsing and wrapperless rendering problem well enough to estimate if I can help fixing and maintaining it.
I can tell you that we will be forced to make it work with React 15, however the whitespace/wrapperless rendering will not be our main focus. We will most likely create a fork of the current develop and fix it that it will work for our needs. Once upgraded to React 16 we will come back to the "official" version of react-jsx-parser.

If you can provide me with insights about why you introduced Fragments I would volunteer to try to find a solution without it.

[TroyAlford]

There are two primary places the Fragment is needed.

First is line 134, where we output the content without a wrapping <div> if the renderInWrapper prop is set to false. Because the rendered JSX may have more than 1 top-level node, we can't just dump out ParsedChildren directly until React 16, where we get the ability for a component to return multiple nodes. This is of no consequence if you don't mind the wrapper <div> being output - you could simply remove the feature for your fork.

The second spot is line 48, where we output the value of a JSXText node. Before this change, the component incorrectly handled values like & and non-breaking spaces. Acorn provides 2 separate values in this case - one of which is the actual symbol (expression.value) and the other is the HTML-encoded version (& or   for instance). Unfortunately, if you simply output   without doing dangerouslySetInnerHTML, React will actually render   - meaning that in the browser, you see the encoded version, not the non-breaking space.

This sucks. Because in our case, there is no element to render with dangerouslySetInnerHTML - because it's a text node. So - there are 3 basic options: 1) inject an extra element, such as a <span>, 2) use a <Fragment>, or 3) rewrite the logic somewhat substantially in order to dangerouslySetInnerHTML on the parent node when parsing a child of this variety.

Option 1 is, imho, patently incorrect - because you are actually changing the HTML being output, which could have unanticipated side-effects based on CSS rules or something. Option 3 felt elusive & over-complicated - so I didn't pursue it. Option 2, Fragment, elegantly solves the problem - and manages to not munge the value.

It's possible that some further exploration, here, would yield you some better fruit than I found - and if so, I'd love to see a different way to solve this icky problem.

The other thing to watch for, if you do so, is the injection of multiple extra nodes. I don't recall exactly what I determined was causing them - but an earlier version of the code would render additional nodes of whitespace inside the component's rendered HTML. If this happens, there are several tests which should fail, showing an unexpected # of child nodes that doesn't match the assertions.

[saendu]

The first issue is indeed not a big problem for us, since we don't mind a wrapper.
The second issue as well is not much of a problem, since we are going to use the parser with the componentsOnly set to true anyways (therefore blocking raw-HTML).

Anyway would you mind providing me with a https://codesandbox.io/ that reproduces the second issue (&)?

[TroyAlford]

Playing with codesandbox a bit - I wonder if your code would just "work" with v1.1.6 of this package. That version is still pinned to React 15.6.1 and doesn't use Fragment at all. It does have the problem of generating extra comments around whitespace nodes - but perhaps that won't matter?

https://codesandbox.io/s/m3kyqq8y2j

[saendu]

Thanks for your suggestion. However we need the latest feature "ComponentsOnly" in order to avoid plain HTML in JSX. This would force us to change the v1.1.6 binaries as well. Therefore I rather fork the v1.3.2 branch and change the code to something like this:

case 'JSXText':
        return (parseInt((React.version).split('.')[0]) >= REACT_FRAGMENT_SUPPORT_VERSION) ? 
               <Fragment key={randomHash()}>{expression.value || ''}</Fragment> : 
                (expression.value || '');

[TroyAlford]

cool - that seems like a reasonable interim solution. :)