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

JSX whitespace coalescing should work like regular HTML #65

Closed
plecong opened this issue Jun 5, 2013 · 12 comments
Closed

JSX whitespace coalescing should work like regular HTML #65

plecong opened this issue Jun 5, 2013 · 12 comments

Comments

@plecong
Copy link

plecong commented Jun 5, 2013

Currently, when two escaped entities are outputted the space between them is removed. For the code below the expected output is "Hello World", but the actual output is "HelloWorld" (without a space between "Hello" and "World").

var Hello = React.createClass({
    render: function() {
        return <div>{this.props.greeting} {this.props.name}</div>;
    }
});

React.renderComponent(<Hello greeting="Hello" name="World" />, document.body);

Link to discussion on Google Groups

@gregrperkins
Copy link
Contributor

+1

Similar to how closure's soy templates do this, it might be useful to retain the whitespace in this instance, where the space is clearly between two elements on a single line.

However, seems ideal to delete all whitespace if the elements are on separate lines, so that:

return <div>
  {this.props.greeting}
  {this.props.name}
</div>

would still be equivalent to <div>{this.props.greeting}{this.props.name}</div> with no space in between.

@bankyadam
Copy link

Ugly, but I use the following until a fix:

var SPACE = ' ';
var MyComponent = React.createClass({
    render: function() {
        return (
            <div>
                {this.props.foo}
                {SPACE}
                {this.props.bar}
            </div>
        );
    }
});

@benjamn
Copy link
Contributor

benjamn commented Jun 11, 2013

@adambrunner yep, that's the only way right now (though I use {' '} directly, myself). This is definitely something we want to fix.

@syranide
Copy link
Contributor

I'm currently looking into amending this and have done some experimentation with good results.

My current implementation basically removes all whitespace that is broken by a newline, and after that all remaining whitespace is collapsed into a single space (and it properly retains whitespace in the produced code).

Basically code can be arbitrarily spread over multiple lines without whitespace being produced, and any explicit whitespace within a line produces a single space (i.e, <div> </div> keeps the whitespace).

"# #"      =>  "# #"
"# \n #"   =>  "##"
"# S #"    =>  "# S #"
"# S\n #"  =>  "# S#"
"S # S"    =>  "S # S"
"S S"      =>  "S S"
"S \n S"   =>  "SS"

# is <tag> and {code}, S is text

To me the output of my current rules seem intuitive, except for the last one, it seems like it should produce a whitespace in the middle (or perhaps not?), and that would be easy to fix. But, are there other things to consider perhaps? Is this change to the implementation acceptable or is remaining 100% compatible more important now?

@sophiebits
Copy link
Collaborator

I agree that those all seem pretty reasonable except the last one. I wouldn't personally find keeping newlines between two tags helpful, though it's at least worth considering because removing it is inconsistent with HTML.

@syranide
Copy link
Contributor

I'm personally perhaps kind of biased in this as I've always run a preprocessor similar to this over all my HTML, because whitespace would otherwise creep in everywhere causing unwanted spacing to appear "randomly". I personally don't know why anyone would ever want to have a newline translate into whitespace.

To be consistent with HTML all whitespace would have to be kept as-is (CSS white-space: pre, etc), but if you ever want that behavior then {' x '} seems like a reasonable solution to me.

Here's an intentionally horrible example along with the code it currently produces with my patch, note that now "S \n S" => "S S".

React.renderComponent(
    <div>    x 
        <span alt="color: #000000" x={123 + 1}></span>w{'awd2'}  {'awd3'}

        o


            l


        {'awd7'}

            {'awd6'}


         z    {'awd4'} 

         w   w   
           {'awd5'}   
         x
    </div>,
    document.getElementById('content')
);

With my fix it produces the following code:
(I intentionally mirror the structure/newlines of the original code, this can be removed if needed)

Thought... perhaps the whitespace between the two w's should be kept as-is, i.e, produce "w<3 spaces>w" and not just "w<1 space>w", seeing as it is probably intentional, as well as for z {'awd4'}. This would be very simple to fix as well.

=> " xwawd2 awd3o lawd7awd6z awd4w wawd5x"

React.renderComponent(
    React.DOM.div(null,     " x", 
        React.DOM.span( {alt:"color: #000000", x:123 + 1}),"w",'awd2',  " ",  'awd3',

        "o "+


            "l",


        'awd7',

            'awd6',


         "z ",    'awd4', 

         "w w",   
           'awd5',   
         "x"
    ),
    document.getElementById('content')
);

This is what JSXTransformer currently produces:
(A lot of, in my opinion, unintended whitespace resulting from newlines as well as "awd2" and "awd3" NOT being separated by whitespace as one would perhaps expect.)

=> " x wawd2awd3 o l awd7awd6 z awd4 w w awd5 x "

React.renderComponent(
    React.DOM.div(null,     " x " ,
        React.DOM.span( {alt:"color: #000000", x:123 + 1}),"w",'awd2',  'awd3',
        " o "+
            "l ",
        'awd7',

            'awd6',
         " z ",    'awd4', 
         " w   w "   ,
           'awd5',   
         " x "
    ),
    document.getElementById('content')
);

@petehunt
Copy link
Contributor

cc @jeffmo

@gregrperkins
Copy link
Contributor

I like where this is going, and agree with your proposed next step: whitespace that does not contain a newline should not be collapsed.

Hopefully since react is not at 1.0 yet, this would still within the range of acceptable compatibility breakages, given a proper writeup in the docs.

@jeffmo
Copy link
Contributor

jeffmo commented Oct 31, 2013

I think I'm onboard with this conceptually. I think this set of rules is, at least, the most intuitive we've come up with thus far. Sounds like everyone else is on board too, so lets do it.

(man, whitespace is a bitch...)

@syranide
Copy link
Contributor

FYI, pull request and more detailed information available at #480

@petehunt
Copy link
Contributor

Is this something we could provide a codemod/recast script for?

When we tweaked this.props.children from always being an array to sometimes being an array it was a huge headache for IG since every callsite had to be manually tested. I want to make sure we don't have to test every callsite and look at whitespace to be sure.

@sophiebits
Copy link
Collaborator

Fixed by #480.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants