Skip to content
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

Loading states api #93

Merged
merged 48 commits into from Feb 28, 2019
Merged

Loading states api #93

merged 48 commits into from Feb 28, 2019

Conversation

valentijnnieman
Copy link
Contributor

@valentijnnieman valentijnnieman commented Oct 30, 2018

Here's the first pass at a loading states api, where dash-renderer now provides a loading_state prop to components that are taking some time to load. It is used with a dist of this PR that provides a Loading component, that can wrap any other Dash component, making it display a loading spinner if it's not fully rendered. An example is included in simple.py, make sure to install the dev-requirements.txt before running it!

TODO:

  • Include which property is loading. Currently we strip the component's name, but it also includes the property that's loading, so we could pass that down as well.
  • Clean-up!

The loading_state prop is an object that looks like:

{
    is_loading: bool,
    component_name: string,
    prop_name: string
}

So that component authors can determine when to show a loading spinner, and know which component in the chain caused the loading to happen (it will return the id of the component), and which prop is loading (for example children).

Community post: https://community.plot.ly/t/loading-states-api-and-a-loading-component-prerelease

src/APIController.react.js Outdated Show resolved Hide resolved
@valentijnnieman valentijnnieman changed the title [WIP] Loading states api Loading states api Nov 6, 2018
@chriddyp
Copy link
Member

chriddyp commented Nov 9, 2018

For those following along in the community, here's a demo:
image
loading states

if (r.status === 'loading' && contains(id, r.controllerId)) {
isLoading = true;
loadingComponent = r.controllerId.split('.')[0];
loadingProp = r.controllerId.split('.')[1];
Copy link
Contributor

@T4rk1n T4rk1n Nov 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can split only one time: [loadingProp, loadingComponent] = r.controllerId.split('.').

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a hard time figuring this out, that is not a proper map. It should be a for loop or a filter or a find.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the most appropriate would be a .forEach()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have been using rambda elsewhere in dash-renderer even when a core javascript function exists for a task, so https://ramdajs.com/docs/#forEach would fit better

}
});

const thisRequest = requestQueue.filter(r => contains(id, r.controllerId));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The filter is here.

package.json Outdated Show resolved Hide resolved
@Marc-Andre-Rivet
Copy link
Contributor

Marc-Andre-Rivet commented Jan 8, 2019

Would like to see the following issues opened as follow ups:

  • enabling direct support of loading state in DCC components (loading_state + animation type)
  • integration testing support for dash-renderer (our tests are all at the behavioural / dash server level atm) -- test loading states, test loading states transition, test more complex graph shapes (Loading component with multiple children, Loading comp within a parent loading comp)
  • removing the 'Loading...' from dash.py

@Marc-Andre-Rivet
Copy link
Contributor

Marc-Andre-Rivet commented Jan 24, 2019

As discussed with @valentijnnieman, we are currently going for a mixed approach, supporting both

  • loading state as a data-attribute on the components + styleguide / css styling of loading comps
  • Loading component with 1 level deep loading_state support -- the user becomes responsible for putting items that are actually slow to load (e.g. no listening to children prop in Dash)

While the updated implementation looks promising, my main concern right now has to do with the mechanics of getting this to work -- the TreeContainer or the NotifyObserver needs to be hooked up to the store's requestQueue through a prop in order to determine the component's state -- the queue is impacted by user actions and changes often during back-and-forth with Dash. Since not all components are pure in html and dcc, or protected by a componentShouldUpdate method, this may cause a significant performance impact. This should be studied and minimized before the feature can be released / sent back for community testing. It's possible that the impact will be negligible but if not, we may have to expose the queue data differently so as to not trigger useless re-render.

@chriddyp
Copy link
Member

the TreeContainer or the NotifyObserver needs to be hooked up to the store's requestQueue through a prop in order to determine the component's state

I'm a little rusty on the redux details here, but I thought that any dispatch that happens will trigger an update. I don't believe that there isn't anything in NotifyObservers that says "If store.x changes, then update. But if store.y changes, don't update.". Does this match your understanding too or am I forgetting about something? If so, can you point me to the code where this happens?

So, if my understanding is right, the existing code is dispatching to update the request queue which is already cause re-rendering. Passing new props through in NotifyObserversComponent shouldn't change this.

Code reference: dispatch(setRequestQueue(

dispatch(setRequestQueue(concat(requestQueue, newRequestQueue)));

Connected NotifyObservers

/*
* NotifyObservers passes a connected `setProps` handler down to
* its child as a prop
*/
function mapStateToProps(state) {
return {
dependencies: state.dependenciesRequest.content,
paths: state.paths,
};
}
function mapDispatchToProps(dispatch) {
return {dispatch};
}
function mergeProps(stateProps, dispatchProps, ownProps) {
const {dispatch} = dispatchProps;
return {
id: ownProps.id,
children: ownProps.children,
dependencies: stateProps.dependencies,
paths: stateProps.paths,
setProps: function setProps(newProps) {
const payload = {
props: newProps,
id: ownProps.id,
itempath: stateProps.paths[ownProps.id],
};
// Update this component's props
dispatch(updateProps(payload));
// Update output components that depend on this input
dispatch(notifyObservers({id: ownProps.id, props: newProps}));
},
};
}
function NotifyObserversComponent({
children,
id,
paths,
dependencies,
setProps,
}) {
const thisComponentSharesState =
dependencies &&
dependencies.find(
dependency =>
dependency.inputs.find(input => input.id === id) ||
dependency.state.find(state => state.id === id)
);
/*
* Only pass in `setProps` if necessary.
* This allows component authors to skip computing unneeded data
* for `setProps`, which can be expensive.
* For example, consider `hoverData` for graphs. If it isn't
* actually used, then the component author can skip binding
* the events for the component.
*
* TODO - A nice enhancement would be to pass in the actual
* properties that are used into the component so that the
* component author can check for something like
* `subscribed_properties` instead of just `setProps`.
*/
const extraProps = {};
if (
thisComponentSharesState &&
// there is a bug with graphs right now where
// the restyle listener gets assigned with a
// setProps function that was created before
// the item was added. only pass in setProps
// if the item's path exists for now.
paths[id]
) {
extraProps.setProps = setProps;
}
if (!isEmpty(extraProps)) {
return React.cloneElement(children, extraProps);
}
return children;
}
NotifyObserversComponent.propTypes = {
id: PropTypes.string.isRequired,
children: PropTypes.node.isRequired,
path: PropTypes.array.isRequired,
};
export default connect(
mapStateToProps,
mapDispatchToProps,
mergeProps
)(NotifyObserversComponent);

@Marc-Andre-Rivet
Copy link
Contributor

@chriddyp No, your understanding seems to be correct. Adding the prop from the store does not have the impact I thought it would have. My thinking was that since N.Obs. had a new prop from the store, that would trigger extra renders but that assumption was incorrect. There are a few extra renders in the observer when first starting the app but after that it's a one to one match with previous behavior.

Thanks for the input.

@valentijnnieman
Copy link
Contributor Author

Released as prerelease version 0.18.0rc1.

@valentijnnieman
Copy link
Contributor Author

There seems to be an issue here that's causing the test_confirm_as_children in dash-core-components to fail. Haven't been able to figure this out yet.

@valentijnnieman
Copy link
Contributor Author

valentijnnieman commented Jan 31, 2019

Published 0.18.0rc3 which should fix some of the tests in DCC failing.

@valentijnnieman
Copy link
Contributor Author

Released as prerelease version 0.18.0rc4

simple.py Outdated Show resolved Hide resolved
@@ -2,6 +2,9 @@
All notable changes to this project will be documented in this file.
This project adheres to [Semantic Versioning](http://semver.org/).

## [0.18.0] - 2019-01-30
### Added
- Loading states API [#267](https://github.com/plotly/dash/issues/267)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For final merge, needs to be unreleased and version bump reverted in package.json

src/TreeContainer.js Outdated Show resolved Hide resolved
return nextProps.layout !== this.props.layout;
return (
nextProps.layout !== this.props.layout
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is checking only for the layout enough to update correctly in all cases? How does this cover the requestQueue updates? Or how is the requestQueue not necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this. The main reason that I haven't added any nextProps.requestQueue checks is that it causes tests to fail. I'm guessing that re-rendering upon differences in the requestQueue prop causes some behaviour to be different - I'm seeing test_radio_buttons_callbacks_generating_children and test_hot_reload tests failing.

src/TreeContainer.js Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants