Merge pull request #22 from plotly/fix-request-order

match response order with request order

Author: chriddyp

Diff for: CHANGELOG.md

@@ -2,13 +2,20 @@
 All notable changes to this project will be documented in this file.
 This project adheres to [Semantic Versioning](http://semver.org/).
 
+## [0.11.0] - 2017-09-28
+### Fixed
+- 🐞 Previously, old requests could override new requests if their response was longer than the new one.
+This caused subtle bugs when apps are deployed on multiple processes or threads with component
+callbacks that update at varying rates like urls. Originally reported in github.com/plotly/dash/issues/133. This fix should also improve performance when many updates happen at once as outdated requests will get dropped instead of updating the UI.
+
 ## [0.10.0] - 2017-09-19
 ### Fixed
 - Fixed an issue where a callback would be fired on page load and when dynamically generated excessively. Previously, the callback would be called as many times as it had inputs. Now, it is called less. https://github.com/plotly/dash-renderer/pull/21
 ### Maintenance
 - Add percy screenshot tests
 
 
+
 ## [0.9.0] - 2017-09-07
 ### Fixed
 - 🐞 Fixed a bug where Dash would fire updates for each parent of a grandchild node that shared the same grandparent. Originally reported in https://community.plot.ly/t/specifying-dependency-tree-traversal/5080/5

Diff for: dash_renderer/version.py

@@ -1 +1 @@
-__version__ = '0.10.0'
+__version__ = '0.11.0'

Diff for: package.json

@@ -1,6 +1,6 @@
 {
   "name": "dash-renderer",
-  "version": "0.10.0",
+  "version": "0.11.0",
   "description": "render dash components in react",
   "main": "src/index.js",
   "scripts": {

Diff for: src/APIController.react.js

@@ -1,7 +1,7 @@
 import {connect} from 'react-redux'
-import {any, contains, equals, isEmpty, isNil} from 'ramda'
+import {contains, isEmpty, isNil} from 'ramda'
 import React, {Component, PropTypes} from 'react';
-import renderTree from './renderTree';
+import TreeContainer from './TreeContainer';
 import {
     computeGraphs,
     computePaths,
@@ -10,7 +10,6 @@ import {
 } from './actions/index';
 import {getDependencies, getLayout} from './actions/api';
 import {APP_STATES} from './reducers/constants';
-import AccessDenied from './AccessDenied.react';
 
 /**
  * Fire off API calls for initialization
@@ -75,24 +74,12 @@ class UnconnectedContainer extends Component {
     render () {
         const {
             appLifecycle,
-            config,
             dependenciesRequest,
-            lastUpdateComponentRequest,
             layoutRequest,
             layout
         } = this.props;
 
-        // Auth protected routes
-        if (any(equals(true),
-                [dependenciesRequest, lastUpdateComponentRequest,
-                 layoutRequest].map(
-            request => (request.status && request.status === 403))
-        )) {
-            return (<AccessDenied config={config}/>);
-        }
-
-
-        else if (layoutRequest.status &&
+        if (layoutRequest.status &&
             !contains(layoutRequest.status, [200, 'loading'])
         ) {
             return (<div>{'Error loading layout'}</div>);
@@ -110,7 +97,7 @@ class UnconnectedContainer extends Component {
         else if (appLifecycle === APP_STATES('HYDRATED')) {
             return (
                 <div id="_dash-app-content">
-                    {renderTree(layout, dependenciesRequest.content)}
+                    <TreeContainer layout={layout}/>
                 </div>
             );
         }
@@ -126,9 +113,7 @@ UnconnectedContainer.propTypes = {
         APP_STATES('HYDRATED')
     ]),
     dispatch: PropTypes.function,
-    config: PropTypes.object,
     dependenciesRequest: PropTypes.object,
-    lastUpdateComponentRequest: PropTypes.objec,
     layoutRequest: PropTypes.object,
     layout: PropTypes.object,
     paths: PropTypes.object,
@@ -139,9 +124,7 @@ const Container = connect(
     // map state to props
     state => ({
         appLifecycle: state.appLifecycle,
-        config: state.config,
         dependenciesRequest: state.dependenciesRequest,
-        lastUpdateComponentRequest: state.lastUpdateComponentRequest,
         layoutRequest: state.layoutRequest,
         layout: state.layout,
         graphs: state.graphs,

Diff for: src/renderTree.js renamed to src/TreeContainer.js

@@ -1,19 +1,33 @@
 'use strict'
 
 import R from 'ramda';
-import React, {PropTypes} from 'react';
+import React, {Component, PropTypes} from 'react';
 import Registry from './registry';
 import NotifyObservers from './components/core/NotifyObservers.react';
 
-export default function render(component) {
+export default class TreeContainer extends Component {
+    shouldComponentUpdate(nextProps) {
+        return nextProps.layout !== this.props.layout;
+    }
+
+    render() {
+        return render(this.props.layout);
+    }
+}
+
+TreeContainer.propTypes = {
+    layout: PropTypes.object,
+}
+
+function render(component) {
     if (R.contains(R.type(component), ['String', 'Number', 'Null'])) {
         return component;
     }
 
     // Create list of child elements
     let children;
 
-    const props = R.propOr({}, 'props', component);
+    const componentProps = R.propOr({}, 'props', component);
 
     if (!R.has('props', component) ||
         !R.has('children', component.props) ||
@@ -34,8 +48,9 @@ export default function render(component) {
         // One or multiple objects
         // Recursively render the tree
         // TODO - I think we should pass in `key` here.
-        children = (Array.isArray(props.children) ? props.children : [props.children])
-                   .map(render);
+        children = (Array.isArray(componentProps.children) ?
+                    componentProps.children : [componentProps.children])
+                    .map(render);
 
     }
 
@@ -60,13 +75,12 @@ export default function render(component) {
     );
 
     return (
-        <NotifyObservers id={props.id}>
+        <NotifyObservers id={componentProps.id}>
             {parent}
         </NotifyObservers>
     );
 }
 
 render.propTypes = {
-    children: PropTypes.object,
-    id: PropTypes.string
+    children: PropTypes.object
 }

Diff for: src/actions/index.js

@@ -1,26 +1,31 @@
 /* global fetch:true, Promise:true, document:true */
 import {
+    __,
+    adjust,
+    any,
     concat,
     contains,
+    findIndex,
+    findLastIndex,
     has,
     intersection,
     isEmpty,
     keys,
     lensPath,
+    merge,
     pluck,
-    reject,
+    propEq,
     slice,
     sort,
     type,
-    union,
     view
 } from 'ramda';
 import {createAction} from 'redux-actions';
 import {crawlLayout, hasId} from '../reducers/utils';
 import {APP_STATES} from '../reducers/constants';
 import {ACTIONS} from './constants';
 import cookie from 'cookie';
-import {urlBase} from '../utils';
+import {uid, urlBase} from '../utils';
 
 export const updateProps = createAction(ACTIONS('ON_PROP_CHANGE'));
 export const setRequestQueue = createAction(ACTIONS('SET_REQUEST_QUEUE'));
@@ -253,10 +258,24 @@ export function notifyObservers(payload) {
              * the change for Child. if this update has already been queued up,
              * then skip the update for the other component
              */
-            const controllersInExistingQueue = intersection(
-                requestQueue, controllers
+            const controllerIsInExistingQueue = any(r =>
+                contains(r.controllerId, controllers) && r.status === 'loading',
+                requestQueue
             );
 
+            /*
+             * TODO - Place throttling logic here?
+             *
+             * Only process the last two requests for a _single_ output
+             * at a time.
+             *
+             * For example, if A -> B, and A is changed 10 times, then:
+             * 1 - processing the first two requests
+             * 2 - if more than 2 requests come in while the first two
+             *     are being processed, then skip updating all of the
+             *     requests except for the last 2
+             */
+
             /*
              * also check that this observer is actually in the current
              * component tree.
@@ -267,7 +286,7 @@ export function notifyObservers(payload) {
              if (
                  (controllersInFutureQueue.length === 0) &&
                  (has(outputComponentId, getState().paths)) &&
-                 (controllersInExistingQueue.length === 0)
+                 !controllerIsInExistingQueue
              ) {
                  queuedObservers.push(outputIdAndProp)
              }
@@ -278,7 +297,20 @@ export function notifyObservers(payload) {
          * updated in a queue. not all of these requests will be fired in this
          * action
          */
-        dispatch(setRequestQueue(union(queuedObservers, requestQueue)));
+        const newRequestQueue = queuedObservers.map(
+            i => ({
+                controllerId: i,
+                status: 'loading',
+                uid: uid(),
+                requestTime: Date.now()
+            })
+        )
+        dispatch(setRequestQueue(
+            concat(
+                requestQueue,
+                newRequestQueue
+            )
+        ));
 
         const promises = [];
         for (let i = 0; i < queuedObservers.length; i++) {
@@ -351,20 +383,71 @@ export function notifyObservers(payload) {
                 credentials: 'same-origin',
                 body: JSON.stringify(payload)
             }).then(function handleResponse(res) {
-                dispatch({
-                    type: 'lastUpdateComponentRequest',
-                    payload: {status: res.status}
-                });
 
-                // clear this item from the request queue
-                dispatch(setRequestQueue(
-                    reject(
-                        id => id === outputIdAndProp,
+                const getThisRequestIndex = () => {
+                    const postRequestQueue = getState().requestQueue;
+                    const requestUid = newRequestQueue[i].uid;
+                    const thisRequestIndex = findIndex(
+                        propEq('uid', requestUid),
+                        postRequestQueue
+                    );
+                    return thisRequestIndex;
+                }
+
+                const updateRequestQueue = rejected => {
+                    const postRequestQueue = getState().requestQueue
+                    const thisRequestIndex = getThisRequestIndex();
+                    const updatedQueue = adjust(
+                        merge(__, {
+                            status: res.status,
+                            responseTime: Date.now(),
+                            rejected
+                        }),
+                        thisRequestIndex,
+                        postRequestQueue
+                    );
+
+                    dispatch(setRequestQueue(updatedQueue));
+                }
+
+                const isRejected = () => {
+                    const latestRequestIndex = findLastIndex(
+                        propEq('controllerId', newRequestQueue[i].controllerId),
                         getState().requestQueue
-                    )
-                ));
+                    );
+                    const rejected = latestRequestIndex > getThisRequestIndex();
+                    return rejected;
+                }
+
+                if (res.status !== 200) {
+                    // update the status of this request
+                    updateRequestQueue(true);
+                    return;
+                }
+
+                /*
+                 * Check to see if another request has already come back
+                 * _after_ this one.
+                 * If so, ignore this request.
+                 */
+                if (isRejected()) {
+                    updateRequestQueue(true);
+                    return;
+                }
 
                 return res.json().then(function handleJson(data) {
+                    /*
+                     * Even if the `res` was received in the correct order,
+                     * the remainder of the response (res.json()) could happen
+                     * at different rates causing the parsed responses to
+                     * get out of order
+                     */
+                     if (isRejected()) {
+                         updateRequestQueue(true);
+                         return;
+                     }
+
+                    updateRequestQueue(false);
 
                     /*
                      * it's possible that this output item is no longer visible.
@@ -467,7 +550,8 @@ export function notifyObservers(payload) {
 
                     }
 
-            })}));
+                });
+            }));
 
         }
 

Diff for: src/components/core/DocumentTitle.react.js

@@ -1,7 +1,7 @@
 /* global document:true */
 
 import {connect} from 'react-redux'
-import {isEmpty} from 'ramda'
+import {any} from 'ramda'
 import {Component, PropTypes} from 'react'
 
 class DocumentTitle extends Component {
@@ -13,7 +13,7 @@ class DocumentTitle extends Component {
     }
 
     componentWillReceiveProps(props) {
-        if (!isEmpty(props.requestQueue)) {
+        if (any(r => r.status === 'loading', props.requestQueue)) {
             document.title = 'Updating...';
         } else {
             document.title = this.state.initialTitle;

Diff for: src/components/core/Loading.react.js

@@ -1,12 +1,12 @@
 import {connect} from 'react-redux'
-import {isEmpty} from 'ramda'
+import {any} from 'ramda'
 import React, {PropTypes} from 'react'
 
 function Loading(props) {
-    if (!isEmpty(props.requestQueue)) {
+    if (any(r => r.status === 'loading', props.requestQueue)) {
         return (
             <div className="_dash-loading-callback"/>
-        )
+        );
     } else {
         return null;
     }

Diff for: src/reducers/api.js

@@ -27,8 +27,5 @@ function createApiReducer(store) {
 }
 
 export const dependenciesRequest = createApiReducer('dependenciesRequest');
-export const lastUpdateComponentRequest = createApiReducer(
-    'lastUpdateComponentRequest'
-);
 export const layoutRequest = createApiReducer('layoutRequest');
 export const loginRequest = createApiReducer('loginRequest');