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

Avoid adding trailing semicolons to inline styles. #9550

Merged
merged 4 commits into from
Jun 5, 2017

Conversation

gunn
Copy link
Contributor

@gunn gunn commented Apr 28, 2017

For #9353.

@syranide
Copy link
Contributor

CSS styles is a hot path and you've changed it from basic string concatenation into multiple array manipulations which should intuitively have a significant impact on performance.

@gunn
Copy link
Contributor Author

gunn commented Apr 28, 2017

Thanks @syranide. I can change it with that in mind. Is there a good system for testing performance?

@aweary
Copy link
Contributor

aweary commented Apr 28, 2017

Could we just keep the existing code and use slice to remove the last character?

return serialized.slice(0, serialized.length - 1) || null

@TryingToImprove
Copy link
Contributor

TryingToImprove commented Apr 29, 2017

Maybe substring would be better..

return serialized ? serialized.substring(0, serialized.length - 2) : null

https://jsperf.com/slice-vs-substr-vs-substring-methods-long-string/2

@aweary aweary requested a review from sophiebits May 1, 2017 18:34
Copy link
Collaborator

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

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

This feels like super overkill and I can't promise we won't go back on this in the future but I guess we can take this change. Instead of calling .substring, can we avoid appending the last semicolon? Something like:

var first = true;
for (...) {
  if (first) {
    first = false;
  } else {
    s += ';';
  }
  s += x + ':' + y;
}

(That's probably faster and might avoid a string copy (depending on the VM), although I'm not in love with the extra branching.)

@aweary feel free to merge after that change

@gunn
Copy link
Contributor Author

gunn commented May 2, 2017

@spicyj How about this?

var delimiter = '';
for (...) {
  s += delimiter + x + ':' + y;
  delimiter = ';';
}

@gunn
Copy link
Contributor Author

gunn commented May 16, 2017

I've updated with that change after comparing approaches here: https://jsperf.com/semicoloning-approaches
Can we merge this now @aweary?

Copy link
Contributor

@aweary aweary left a comment

Choose a reason for hiding this comment

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

Looks great, good work @gunn! Very elegant solution.

@aweary aweary merged commit 5aa3153 into facebook:master Jun 5, 2017
@sebmarkbage
Copy link
Collaborator

@aweary This failed CI.

@aweary
Copy link
Contributor

aweary commented Jun 5, 2017

@sebmarkbage hmm, it shows that CI passed on my end

screen shot 2017-06-05 at 4 01 24 pm

@gaearon gaearon mentioned this pull request Jun 5, 2017
@gaearon
Copy link
Collaborator

gaearon commented Jun 5, 2017

#9854

@gaearon
Copy link
Collaborator

gaearon commented Jun 7, 2017

Here's why CI failed on master #9864 (comment)

@dustin-page
Copy link

dustin-page commented Dec 3, 2018

I think this may need to be re-opened. I am getting a Warning because the Client is missing the trailing semi-colon and there are no spaces between the CSS properties and values.

In our development environment we are not minifying the HTML code. We only minify in production.

warning.js:33 Warning: Prop style did not match. Server: "animation-duration: 3s; height: 123px;" Client: "animation-duration:3s;height:123px"

@syranide
Copy link
Contributor

syranide commented Dec 3, 2018

@dustin-page You should not be minifying React-generated HTML, it can affect the visual rendering.

@dustin-page
Copy link

@syranide Thanks for the tip regarding production minification of HTML.

The warning I'm seeing is in development with the string concatenation in the createDangerousStringForStyles method. The HTMLElement.style returns "animation-duration: 3s; height: 123px;" and calling createDangerousStringForStyles on line 7910 of the react-dom.development.js file returns a string without spaces or the trailing semicolon: "animation-duration:3s;height:123px". I believe the createDangerousStringForStyles should return a string with spaces and a trailing semicolon.

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

Successfully merging this pull request may close these issues.

9 participants