-
Notifications
You must be signed in to change notification settings - Fork 95
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
[performance] Cache by the resulting WithStyles
HOC
#234
Conversation
Cache by WithStyles component so that we avoid unnecessarily calling the create function of aphrodite.
fbb49c4
to
1f0454f
Compare
f560516
to
e1d3671
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing!
src/withStyles.jsx
Outdated
getCurrentInterface() { | ||
// Fallback to the singleton implementation | ||
return (this.context && this.context.stylesInterface) || _getInterface(); | ||
return ( | ||
(this.context && this.context.stylesInterface) || _getInterface() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nbd ofc but this change seems surprising. is your editor running more than eslint autofix on things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ljharb You're right this is weird, lol. I have some weird linting/prettier/editor autofix stuff going on 🤦♀ I tried fixing it but it's still giving me things like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally the only formatting you want enabled is eslint autofix - ie, in vscode, disable "format on save", and prettier, and be sure you don't have any global eslint installed (so it always uses the local one)
Summary
Cache even further by the resulting
WithStyles
component so that we avoid calling create/resolve unnecessarily per component instance. Before, we were calling create once per theme, per component instance, per direction. This way, we're calling create once per theme, per component definition, per direction, which should happen less often.We also refactor some class methods that didn't need to be class methods
I would especially appreciate feedback around the organization of this code for performance and clarity. Take a look at the commits for easy of review.
Reviewers
@esprehn @TaeKimJR @joeuy @indiesquidge @ljharb @marsjosephine