perf(useFela): optimize passing of theme & props #674
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Not sure why tests are failing in the CI env, they all pass locally |
In many simple use cases, the `useFela` hook will be used without any
props passed to it, and will only make the `theme` available to the
rule callbacks in the returned `css` function. In light of that,
this commit makes two optimizations:
1. Only merge props into the object that is passed to `css`'s
callbacks when props are actually passed.
2. Merge props into the `propsWithTheme` object using mutative
`Object.assign`, as it is by far the most performant way of doing
so. See: https://jsperf.com/object-assign-vs-spreading/3
These may seem like micro optimizations, but since useFela is a hot
path that could potentially run thousands of times, I think this is
of value in this case.
Throw an error with a helpful message when the `useFela` hook is used in a component that isn't a descendant of a RendererProvider
TxHawks
force-pushed
the
perf/useFela-props
branch
from
c6f291f
to
b6293d2
Compare
TxHawks
changed the title
|
Sorry, my bad. Fixed now |
robinweser
approved these changes
Feb 20, 2019
|
I hate mutating like that too, but it's an object we create and completely control, so the perf gains in a hot-path seem worth it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This pull requests makes the following suggestions
Optimizations
In many simple use cases, the
useFelahook will be used without any props passed to it, and will only make thethemeavailable to the rule callbacks in the returnedcssfunction. In light of that,this commit makes two optimizations:
css's callbacks when props are actually passed.propsWithThemeobject using mutativeObject.assign, as it is by far the most performant way of doing so. See: https://jsperf.com/object-assign-vs-spreading/3These may seem like micro optimizations, but since useFela is a hot
path that could potentially run thousands of times, I think this is
of value in this case.
DX
The
useFelahook now throws an error with a helpful message when it is used in a component that isn't a descendant of aRendererProviderPackages
react-fela
Versioning
Patch
Checklist
Quality Assurance
yarn run format)yarn run lint)yarn run test)yarn run flow)Changes
If one of the following checks doesn't make sense due to the type of PR, just check them.