Change renderer setProps assignation logic
#126
Conversation
- always update self - only update Dash if an updated prop is listened to
| } | ||
| return children; | ||
| function NotifyObserversComponent({ children, setProps }) { | ||
| return React.cloneElement(children, { setProps }); |
There was a problem hiding this comment.
Always pass the setProps function to the component. It will censure itself as needed.
| dependency.inputs.find(input => input.id === ownProps.id && input.property === key) || | ||
| dependency.state.find(state => state.id === ownProps.id && state.property === key) | ||
| ) | ||
| )(keysIn(newProps)); |
There was a problem hiding this comment.
From all the props updated, find the ones listened for
| id: ownProps.id, | ||
| props: pick(watchedKeys)(newProps) | ||
| })); | ||
| } |
There was a problem hiding this comment.
Only dispatch to Dash if at least one watched prop is updated
| itempath: stateProps.paths[ownProps.id], | ||
| }; | ||
| itempath: stateProps.paths[ownProps.id] | ||
| })); |
There was a problem hiding this comment.
The component used to only be updated when it had a watched prop -- this makes no sense as it prevents components from updating themselves and have consistent behavior in the FE in all usage scenarios.
|
This looks like a good idea to me, but I'm not sure I'm aware of all the possible implications. @T4rk1n would you mind taking a look? |
Marc-Andre-Rivet
changed the title
setProps assignation logicsetProps assignation logic
I think this implementation makes sense. I originally made " In this PR, if you're only re-rendering the component if it is part of a callback, then there shouldn't be any adverse performance effects 👌 On another note, from a documentation point of view, we'll want to update our 'react for python devs' guide (https://dash.plot.ly/react-for-python-developers) as well as the boilerplate. Should mostly just be a lot of 🔪 though 😸 |
# Conflicts: # src/components/core/NotifyObservers.react.js
- memoize + PureComponent TreeContainer
|
|
src/TreeContainer.js
Outdated
| isEqualArgs(lastArgs, args) ? | ||
| lastResult : | ||
| (lastArgs = args) && (lastResult = fn(...args)); | ||
| } |
There was a problem hiding this comment.
memoize and equality code copied over from the table for experimenting
…into set-props-callback
|
Simplified the changes -- since the renders are mostly driven through changes to immutable values in the store, PureComponents + memoization actually provides no benefit. Prepended renderer centric props with |
Marc-Andre-Rivet
changed the title
setProps assignation logicsetProps assignation logic
|
@alexcjohnson I think this is ready for another look. |
There was a problem hiding this comment.
I'm happy with this 💃
@chriddyp did you still want to try it on a big app?
I shared the source of a private large app with @Marc-Andre-Rivet to test things out with. I'm 💃 either way. Also note that there are a few places in the documentation that we should update once this is released:
|
|
Did some actual performance tests, mostly against the dash-docs. Got the demo app working partially but against the latest version of dash but there were still multiple issues, fixed at lot of them and got the code running but in a weird state -- and not sure how significant the results really are. In all cases, opening a new browser window for each version, the runs are all in the same tab. Version ordering changes randomly. One warm up run prior to taking results. In the docs' dcc page, simply reloading the page gave the following results in milliseconds for 5 reloads: Table paging page, clicking prev/nex 10 times each, in milliseconds: Table editing, modifying 10 cells per run, in milliseconds: The Graph example w/ hover, 20 hovers, in milliseconds: The Graph example w/ hover, 5 page loads, in milliseconds: It seems that at worse this has no impact for certain components (e.g. Graph) and that at best, for certain interactions the performance impact can be very significant. |
|
Nice, those look encouraging. Another page I just thought of is https://dash.plot.ly/all. It's a "hidden" page that we used to generate a PDF version of the docs. It loads all of the pages in the docs at once (takes like 40 seconds to load), probably the most render intensive page that I can think of. |
|
Dry run from a new tab of Dash 0.39: "Updating..." disappeared after ~ 229 seconds Modified: "Updating..." disappeared after 17.2, 17.4 and 18.1 seconds |
|
2 step merge -- will remove the refs for the feature branches of dcc and html right after tests pass everywhere |
Fixes #40.
(Supersedes) Fixes https://github.com/plotly/dash-docs/issues/352.
(Supersedes) Fixes https://github.com/plotly/dash-docs/issues/435
Currently
setPropsis passed to the components only if a component property is used as an input or a state in a callback. This causes two problems:components do not update as expected as the props inside the component itself are only updated if setProps exists:
here we set the own-props
dash-renderer/src/components/core/NotifyObservers.react.js
Line 39 in 91084ce
here we decide if there's a
setPropsdash-renderer/src/components/core/NotifyObservers.react.js
Line 86 in 91084ce
the table is in its own special land as it loops back on itself if it doesn't have a setProps (uses its own internal state to update itself) -- which is fine until the following happens: a callback updates the table AND no callback requires a table prop as input / state -- the table now updates itself into its state and receives updates into its props -- for reasons not detailed here, the table merges states unto props, overriding props in the process -- the net result is that the props update from the callbacks are overwritten by the table's state as in updating a datatable with editable=True dash-table#386
(the table will still need its loopback with this fix for standalone purposes)
Additionally, the current renderer logic is very permissive as to which props will be sent. If a prop is used as input / state, each time a prop is updated, all updated props will be sent to Dash. For most components the overhead is a minor nuisance but for the table, updating
dataand sending it back to Dash, if not required, is a major overhead, possibly orders of magnitude larger than the useful data.This PR proposes a solution to both problems:
setPropsto the componentsetProps: (1) always update the component itself -- no more stale updates that are impossible to understand for users -- this is seriously probably the most frequent question I answer with "create a bogus callback on your prop as a temporary solution", (2) filter out the props that are not asked for by DashProblems that arise from this seem to mostly be related to the way we test..
Performance: #126 (comment)
Companion PRs
plotly/dash-core-components#478
plotly/dash-html-components#99
https://github.com/plotly/dash-docs/pull/439
plotly/dash-component-boilerplate#65