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

create-emotion-server: refactor inline for performance #725

Merged
merged 1 commit into from
Jun 14, 2018

Conversation

apapirovski
Copy link
Contributor

@apapirovski apapirovski commented Jun 14, 2018

The current version of renderStylesToString is highly inefficient as it allocates new arrays over and over, which isn't necessary. Instead just do a single pass hydrate which improves performance roughly 30x in most scenarios.

We're using emotion at Postmates and this was a significant bottleneck to our SSR. (This change, which we're currently floating, took most of our style hydrates from 25-30ms to sub-1ms.)

I tested this by comparing the output of several pages and both the new and old produce identical markup. It doesn't seem like the tests in the library itself are quite detailed enough.

Checklist:

  • Documentation N/A
  • Tests N/A
  • Code complete

The current version of renderStylesToString is highly inefficient as
it allocates new arrays over and over, which isn't necessary.
Instead just do a single pass hydrate which improves performance
roughly 30x in most scenarios.
@codecov
Copy link

codecov bot commented Jun 14, 2018

Codecov Report

Merging #725 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted Files Coverage Δ
packages/create-emotion-server/src/inline.js 100% <100%> (ø) ⬆️

Copy link
Member

@Andarist Andarist left a comment

Choose a reason for hiding this comment

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

Seems like a no-brainer to me 👍 great stuff 😍

would u be able to provide a simple benchmark for this? would be super cool to track this over time to prevent potential future regressions

Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

Thanks!!

@emmatown emmatown merged commit 0847bd2 into emotion-js:master Jun 14, 2018
@apapirovski
Copy link
Contributor Author

would u be able to provide a simple benchmark for this? would be super cool to track this over time to prevent potential future regressions

If I get some time, I'll definitely make a PR for those in the future. 👍

@apapirovski apapirovski deleted the ssr-inline-perf branch June 15, 2018 16:48
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.

3 participants