search

duncanbeevers / jade-react

Compile Jade templates to React.DOM expressions
152 stars 9 forks source link

Don't do concatenation #1

Closed zpao closed 10 years ago

zpao commented 10 years ago

Concatenation is very much the wrong thing to do with siblings. You should instead be creating arrays.

ul
  li List Item
  li= dynamicListItem

is converting to (with newlines to aid reading)

React.DOM.ul(null,
  React.DOM.li(null, "List Item") +
  React.DOM.li(null, dynamicListItem)
)

but it should convert to

React.DOM.ul(null, [
  React.DOM.li(null, "List Item"),
  React.DOM.li(null, dynamicListItem)
])
duncanbeevers commented 10 years ago

@zpao Thanks. I knocked this out as a proof-of-concept and only just started wiring it up into a project.

It looks like I don't even need to wrap these in array, but can just pass them in as successive args.

React.DOM.ul(null,
  React.DOM.li(null, "List Item"),
  React.DOM.li(null, dynamicListItem)
)

I'll poke around with this later tonight, or feel free to send a PR.

duncanbeevers commented 10 years ago

Okay, this commit fixes the concatenation issue by generating args instead. It looks like JSX requires a root node.

@zpao Do you have any suggestions for a fallback strategy? I could force a shim node, but my inclination is just to make the compiler complain.

zpao commented 10 years ago

I won't have time to send any PRs, sorry. And honestly, I don't like Jade syntax, just want to help you help others who might :)

I think letting React complain is fine. We do the same with JSX. It keeps the transform simpler and forces the issue up to the developer instead of magically inserting nodes.

duncanbeevers commented 10 years ago

Thanks for the feedback, really appreciated.