-
Notifications
You must be signed in to change notification settings - Fork 525
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
Memoize getCalculatedProps #2006
Conversation
@@ -1,8 +1,9 @@ | |||
/* eslint-disable func-style */ | |||
/* eslint-disable no-use-before-define */ | |||
import { assign } from "lodash"; | |||
import { assign, omit } from "lodash"; |
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.
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
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.
Will do!
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.
@beccanelson this looks great. I left one minor piece of feedback, but I think this is otherwise good to go.
@@ -168,7 +168,7 @@ const withoutSharedEvents = (props) => { | |||
const modifiedChildren = React.Children.toArray(children).map((child) => { | |||
return { | |||
...child, | |||
props: omit(child.props, "sharedEvents") | |||
props: Helpers.omit(child.props, ["sharedEvents"]) |
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.
I think you'll have an unused lodash omit
in this file now
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.
Yep, I just fixed that
🚀 |
This makes a small performance improvement or VictoryGroup and VictoryStack by memoizing the results of the calculated props so that the
reduceChildren
function 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.getChildren
andComponent.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
sharedEvents
gets 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 accesssharedEvents
from 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.