Experiment/scu #272
Experiment/scu #272
Conversation
|
awesome. from what I can see it is now functionally correct (unlike my attempt). but you're right, the heavily lifting is still quite heavy. Are we copying |
|
|
|
@chrisbolin it's still a noticeable improvement over master for large datasets. I'm not sure how to get around the slow step. |
| { name: "parent", index: "parent" }, { name: "data" }, { name: "labels" } | ||
| ]; | ||
|
|
||
| const areVictoryPropsEqual = (a, b) => { |
There was a problem hiding this comment.
Something like this should have a ton of unit tests behind it. A recursive equality check is something that it's so easy to have incorrect edge cases with.
Perhaps ticket out for "soon" if not now?
/cc @boygirl @chrisbolin
| return undefined; | ||
| } else { | ||
| return component.index !== undefined ? | ||
| getState(component.index, component.name) : |
| // don't check for changes on parent props for non-standalone components | ||
| return undefined; | ||
| } else { | ||
| return component.index !== undefined ? |
There was a problem hiding this comment.
NIT: typeof FOO === "undefined" or _.isUndefined()
| this.hasEvents = props.events || props.sharedEvents || this.componentEvents; | ||
| this.events = this.getAllEvents(props); | ||
| const baseProps = this.getBaseProps(props, getSharedEventState); | ||
| const dataKeys = Object.keys(baseProps).filter((key) => key !== "parent"); |
There was a problem hiding this comment.
You're importing lodash _.keys elsewhere. Perhaps decide on one canonical way to do it? (Unless there's some reason for _.keys above that Object.keys can't do).
| const sharedParentState = this.getSharedEventState("parent", "parent"); | ||
| cacheValues(obj) { | ||
| keys(obj).forEach((key) => { | ||
| this[key] = obj[key]; |
There was a problem hiding this comment.
Are there any collision protections? It seems like you're adding straight to the underlying object. E.g., if someone passes an object with:
{ cacheValues: "foo" }it would blow away this version method of this.cacheValues()
| @@ -125,7 +125,7 @@ export default { | |||
| const mutationKeys = getKeys(childName); | |||
| return Array.isArray(mutationKeys) ? | |||
| mutationKeys.reduce((memo, key) => { | |||
| return assign(memo, getMutationObject(key, childName)); | |||
| return assign({}, memo, getMutationObject(key, childName)); | |||
There was a problem hiding this comment.
Why aren't we mutating memo? Seems like extra work to copy all the props for no benefit (?)
| @@ -134,7 +134,7 @@ export default { | |||
| const allChildNames = childNames === "all" ? | |||
| without(Object.keys(baseProps), "parent") : childNames; | |||
| return Array.isArray(allChildNames) ? allChildNames.reduce((memo, childName) => { | |||
| return assign(memo, getReturnByChild(childName)); | |||
| return assign({}, memo, getReturnByChild(childName)); | |||
cc/ @chrisbolin
This was the most fool-proof way I could figure out to prevent unnecessary re-renders...
sCU logic is roughly:
animating ? true
state changes* ? true
props changes* ? true
else false
state changes:
calculates all the aggregate state changes that could affect a given component in sCU and compares them to the existing cached state changes in a deep equality check. State changes on the
parentnamespace are ignored when the component is not standalone.props changes:
prior to a deep equality check on props, and functions are removed. We were seeing unnecessary re-renders because isEqual doesn't support checking functions, and always returns false when comparing functions.
This change requires adding
optionsto non-standard Victory components (i.e. axes and continuous components likeVictoryArea) so that the state check know which types / indices to check i.e. "data", "labels", "tickLabels" etc.See: FormidableLabs/victory-chart#500 (whoa, 500!)
This change is still slower than your WIP branch, but hits all the edge cases that were missing, specifically updating parent states (i.e. stacked bar example in victory voronoi container demo). I'm seeing what can be done to speed it up.