Memoize getCalculatedProps #2006
Merged
Conversation
This file contains hidden or 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
ryan-roemer
reviewed
Oct 22, 2021
ryan-roemer
reviewed
Oct 22, 2021
added 2 commits
October 22, 2021 12:19
boygirl
reviewed
Oct 25, 2021
| /* eslint-disable func-style */ | ||
| /* eslint-disable no-use-before-define */ | ||
| import { assign } from "lodash"; | ||
| import { assign, omit } from "lodash"; |
Contributor
There was a problem hiding this comment.
Victory maintains a simplified omit helper that is more performant than the more complete lodash.omit. You might consider using that instead: https://github.com/FormidableLabs/victory/blob/main/packages/victory-core/src/victory-util/helpers.js#L31
boygirl
approved these changes
Oct 25, 2021
Contributor
boygirl
left a comment
boygirl
left a comment
There was a problem hiding this comment.
@beccanelson this looks great. I left one minor piece of feedback, but I think this is otherwise good to go.
boygirl
reviewed
Oct 27, 2021
| return { | ||
| ...child, | ||
| props: omit(child.props, "sharedEvents") | ||
| props: Helpers.omit(child.props, ["sharedEvents"]) |
Contributor
There was a problem hiding this comment.
I think you'll have an unused lodash omit in this file now
Contributor
Author
There was a problem hiding this comment.
Yep, I just fixed that
Contributor
|
🚀 |
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.
This makes a small performance improvement or VictoryGroup and VictoryStack by memoizing the results of the calculated props so that the
reduceChildrenfunction is called fewer times.This function is still called at least once on every mouseover event in a
VictoryVoronoiContainer. Victory currently relies heavily on static properties likeComponent.getChildrenandComponent.defaultEvents. I am finding these static functions to be a lot harder to memoize, and it might be worth re-evaluating this architecture in the future.Another thing that made this tricky was the way that
sharedEventsgets passed into every data component as a prop. In order to get memoization working here, I needed to go through and strip this prop from the children so thatisEqual(props, newProps)only returns false if the actual data has changed. I would also recommend updating this pattern in the future so that child components can accesssharedEventsfrom shared context or state rather than receiving it as a prop. When the props are manipulated on each render, it causes a lot of extra re-rendering as the parent components perceive that their children have all changed, even if the shared state only affects one component (like the tooltip).I'll leave #1996 open for now, since there is still some additional work to be done here.