Skip to content

JSXTransformer, concatenate consecutive strings #489

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

Closed
wants to merge 1 commit into from
Closed

JSXTransformer, concatenate consecutive strings #489

wants to merge 1 commit into from

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.

function renderXJSExpressionContainer(traverse, object, isLast, path, state) {
function renderXJSExpressionContainer(
traverse, object, isLast, path, state, concat)
{
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