Skip to content

Conversation

@syranide
Copy link
Contributor

@syranide syranide commented Nov 7, 2013

JSXTransformer currently produces suboptimal code when fixed strings are inserted with braces, {'Abc'} or {"Abc"}, as is required if you want to override the JSX whitespace collapsing.

This means that <div>A{'\n'}B</div> does not generate 1 div, but 1 div and 3 spans <div><span>A</span><span>\n</span><span>B</span></div> rather than just <div><span>A\nB</span></div>.

My proposed changes treats {'Abc'} and {"Abc"}, and ONLY those as if they were actually written inline without braces, but after whitespace collapsing has occured, so their content is exactly what ends up in the DOM. To clarify {StringVar}, {'A'+'B'} and even {('Abc')} are not transformed, so this will have no effect on runtime keying.

If my PR for stricter whitespace collapsing rules is accepted, this will hopefully be less of an issue. But still, a single {'\n'} can cause 0 spans to become 3 which can be a heavy price considering the DOM is the costly part, and this is part of the JSXTransformer which in my opinion, should make a reasonable effort to generate "fast code" as long as it doesn't affect readability.

I also removed a duplicated piece of code (I can move it into it's own PR), because it simplifies the implementation of this PR.

Example

<div>
  abc{'\n'}def
  def{" \t"}ghi
  ghi{Var}jkl
  jkl{' '+Var}mno
</div>

Before

Although a contrived example, this will end up creating 9 spans.

React.DOM.div(null, 
  'abc','\n','def '+
  'def'," \t",'ghi '+
  'ghi',Var,'jkl '+
  'jkl',' '+Var,'mno'
)

After

After fixed strings have been concatenated, only 5 spans will be generated. Note that the two expressions with Var have not been concatenated, but remains as arguments, and as such still have their own spans.

React.DOM.div(null, 
  'abc'+'\n'+'def '+
  'def'+" \t"+'ghi '+
  'ghi',Var,'jkl '+
  'jkl',' '+Var,'mno'
)

@petehunt
Copy link
Contributor

petehunt commented Nov 7, 2013

I am a bit unsure about this fix since it adds an additional case to think about with respect to keying. So I would be slightly inclined to not do this, but I don't feel very strongly.

@sophiebits
Copy link
Collaborator

I had a similar reaction.

@syranide
Copy link
Contributor Author

syranide commented Nov 7, 2013

@petehunt @spicyj At the request of Pete Hunt I have updated the main post and (hopefully) further clarified the scope of my change. To sum it up, ONLY {'Abc'} and {"Abc"} are candidates for concatenation, anything even slightly deviating from that such as {('Abc')} is not concatenated, so this has no runtime implication with respect to keying. This change, in my opinion, ensures that JSXTransformer makes a reasonable effort not to produce suboptimal code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style here would be

function name(
  args, args, args
) {
  code;

But other than that I'm going to step back and let you work with @jeffmo again

@syranide
Copy link
Contributor Author

@jeffmo Gooooood neeeeeews everyone!

@spicyj is awesome and PRed this #742, which solves this in React and potentially opens up for doing away with spans completely in the future.

It doesn't invalidate this PR, I would still consider this a quality/performance improvement for the JSX output. But this PR no longer in any way affects the final output. I still recommend this PR for those reasons, if you don't consider the code bloat too significant (and I wouldn't object if you do).

@ghost ghost assigned jeffmo Dec 31, 2013
@syranide syranide closed this Jan 22, 2014
@syranide syranide deleted the jsxconcat branch January 22, 2014 22:00
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

Successfully merging this pull request may close these issues.

5 participants