acornjs / acorn-jsx

Alternative, faster React.js JSX parser
MIT License
646 stars 72 forks source link

JSX whitespace issue #13

Closed gaearon closed 9 years ago

gaearon commented 9 years ago

This is a repost of https://github.com/6to5/6to5/issues/419.

var Foo = React.createClass({
  render() {
    return (
      <span>foo</span>
    );
  }
});

var Bar = React.createClass({
  render() {
    return (
      <div>
        <Foo/> bar
      </div>
    );
  }
});

The whitespace between <Foo/> and bar is missing in the output.

gaearon commented 9 years ago

Is there any kind of quick temporary fix for this that wouldn't necessitate the rewrite?

I'd really like this one case fixed, as this is what prevents us from using 6to5, and I already changed build process to use 6to5.

I'm happy to work on it if you can point me in the right direction.

RReverser commented 9 years ago

As a quick one on your side, you can use method from early days of JSX when it didn't support whitespace output at all - just insert {' '} in your code.

I mentioned rewrite because this issue raises up again and again. We had the same with ES6 templates earlier too, and without integrating the same fix, each case requires own step-by-step debugging of simplified reproducible test case just to find where and why it happens. If you want - you can do that on your side now and I'll be happy to accept PR.

gaearon commented 9 years ago

As a quick one on your side, you can use method from early days of JSX when it didn't support whitespace output at all - just insert {' '} in your code.

Yeah I was considering that but it's hard to find places in the source code where I might need this, as I have hundreds of components.

If you want - you can do that on your side now and I'll be happy to accept PR.

I think I'll give it a shot if I have time, and if I find some cheap way to do that, I'll ping you back.

Thanks!

gaearon commented 9 years ago

OTOH grepping for \w>[ ] in JSX files should probably be enough.

gaearon commented 9 years ago

BTW this only seems to happen after self-closing tags.

RReverser commented 9 years ago

Fixed with rewrite, I'm just willing to wait for some upstream changes and merge them before release. /cc @sebmck

sebmck commented 9 years ago

@RReverser Awesome! What about #14? It's a pretty serious issue as it currently breaks source maps.

RReverser commented 9 years ago

@sebmck Just answered there :)

RReverser commented 9 years ago

@sebmck But I'd ask not to merge this to 6to5 until I finish some core changes in upstream (want to be sure that nothing will be broken).

sebmck commented 9 years ago

@RReverser Sure, I'll wait for your go-ahead.

gaearon commented 9 years ago

Really appreciate you tackling this! Thank you for taking time.

RReverser commented 9 years ago

Always welcome!

gaearon commented 9 years ago

:+1:

RReverser commented 9 years ago

@sebmck I guess we are now good to go. Please note that prefixes are now changed to JSX instead of legacy XJS (see facebook/esprima#83 for context). Let me know about any issues you encounter after upgrade - I'll be happy to fix 'em.

sebmck commented 9 years ago

@RReverser Great, thank you! Few merge conflicts but nothing serious, passes the 6to5 and acorn-6to5 test suite so all is good, thanks again!

RReverser commented 9 years ago

@sebmck Great to hear!