diff --git a/CHANGELOG.md b/CHANGELOG.md index d92d560..0d95d7e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ All notable changes to this project will be documented in this file. This project adheres to [Semantic Versioning](http://semver.org/). +## [0.11.3] - 2018-02-01 +### Fixed +- Fixed #41 in #42. In some cases, during initialization, callbacks may fired multiple times instead of just once. This only happens in certain scenarios where outputs have overlapping inputs and those inputs are leaves (they don't have any inputs of their own). See #41 for a simple example and #42 for some more extensive test cases. +- Fixed #44 in #42. If an output component is returned from a callback and its inputs were _not_ returned from the same input (i.e. they were already visible), then the callback to update the output would not fire. This has now been fixed. A common scenario where this app structure exists is within a Tabbed app, where there are global controls that update each tab's contents and the tab's callback just displays new output containers. See #44 for a simple example and #42 for some more extensive test cases. + ## [0.11.2] - 2018-01-08 ### Fixed - Removes logging from redux middleware from production build based on process.env.NODE_ENV. diff --git a/dash_renderer/version.py b/dash_renderer/version.py index 2b3823f..4ad2f68 100644 --- a/dash_renderer/version.py +++ b/dash_renderer/version.py @@ -1 +1 @@ -__version__ = '0.11.2' +__version__ = '0.11.3' diff --git a/package.json b/package.json index abbf8b7..74167a5 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "dash-renderer", - "version": "0.11.2", + "version": "0.11.3", "description": "render dash components in react", "main": "src/index.js", "scripts": { diff --git a/src/actions/index.js b/src/actions/index.js index 5f384c8..d570cc0 100644 --- a/src/actions/index.js +++ b/src/actions/index.js @@ -3,10 +3,13 @@ import { __, adjust, any, + append, concat, contains, findIndex, findLastIndex, + flatten, + flip, has, intersection, isEmpty, @@ -15,9 +18,11 @@ import { merge, pluck, propEq, + reject, slice, sort, type, +// values, view } from 'ramda'; import {createAction} from 'redux-actions'; @@ -42,7 +47,6 @@ export function hydrateInitialOutputs() { } } - function triggerDefaultState(dispatch, getState) { const {graphs} = getState(); const {InputGraph} = graphs; @@ -57,15 +61,15 @@ function triggerDefaultState(dispatch, getState) { * and the invisible inputs */ if (InputGraph.dependenciesOf(nodeId).length > 0 && - InputGraph.dependantsOf(nodeId).length == 0 && + InputGraph.dependantsOf(nodeId).length === 0 && has(componentId, getState().paths) ) { inputNodeIds.push(nodeId); } }); - reduceInputIds(inputNodeIds, InputGraph).forEach(nodeId => { - const [componentId, componentProp] = nodeId.split('.'); + reduceInputIds(inputNodeIds, InputGraph).forEach(inputOutput => { + const [componentId, componentProp] = inputOutput.input.split('.'); // Get the initial property const propLens = lensPath( concat(getState().paths[componentId], @@ -78,8 +82,10 @@ function triggerDefaultState(dispatch, getState) { dispatch(notifyObservers({ id: componentId, - props: {[componentProp]: propValue} + props: {[componentProp]: propValue}, + excludedOutputs: inputOutput.excludedOutputs })); + }); } @@ -135,7 +141,9 @@ function reduceInputIds(nodeIds, InputGraph) { */ const inputOutputPairs = nodeIds.map(nodeId => ({ input: nodeId, - outputs: InputGraph.dependenciesOf(nodeId) + // TODO - Does this include grandchildren? + outputs: InputGraph.dependenciesOf(nodeId), + excludedOutputs: [] })); const sortedInputOutputPairs = sort( @@ -143,12 +151,28 @@ function reduceInputIds(nodeIds, InputGraph) { inputOutputPairs ); - const uniquePairs = sortedInputOutputPairs.filter((pair, i) => !contains( - pair.outputs, - pluck('outputs', slice(i + 1, Infinity, sortedInputOutputPairs)) - )); + /* + * In some cases, we may have unique outputs but inputs that could + * trigger components to update multiple times. + * + * For example, [A, B] => C and [A, D] => E + * The unique inputs might be [A, B, D] but that is redudant. + * We only need to update B and D or just A. + * + * In these cases, we'll supply an additional list of outputs + * to exclude. + */ + sortedInputOutputPairs.forEach((pair, i) => { + const outputsThatWillBeUpdated = flatten(pluck( + 'outputs', slice(0, i, sortedInputOutputPairs))); + pair.outputs.forEach(output => { + if (contains(output, outputsThatWillBeUpdated)) { + pair.excludedOutputs.push(output); + } + }); + }); - return pluck('input', uniquePairs); + return sortedInputOutputPairs; } @@ -158,19 +182,15 @@ export function notifyObservers(payload) { const { id, event, - props + props, + excludedOutputs } = payload const { - config, - layout, graphs, - paths, requestQueue, - dependenciesRequest } = getState(); const {EventGraph, InputGraph} = graphs; - /* * Figure out all of the output id's that depend on this * event or input. @@ -195,6 +215,13 @@ export function notifyObservers(payload) { }); } + if (excludedOutputs) { + outputObservers = reject( + flip(contains)(excludedOutputs), + outputObservers + ); + } + if (isEmpty(outputObservers)) { return; } @@ -317,267 +344,361 @@ export function notifyObservers(payload) { const outputIdAndProp = queuedObservers[i]; const [outputComponentId, outputProp] = outputIdAndProp.split('.'); - /* - * Construct a payload of the input, state, and event. - * For example: - * If the input triggered this update, then: - * { - * inputs: [{'id': 'input1', 'property': 'new value'}], - * state: [{'id': 'state1', 'property': 'existing value'}] - * } - * - * If an event triggered this udpate, then: - * { - * state: [{'id': 'state1', 'property': 'existing value'}], - * event: {'id': 'graph', 'event': 'click'} - * } - * - */ - const payload = { - output: {id: outputComponentId, property: outputProp} - }; + const requestUid = newRequestQueue[i].uid; - if (event) { - payload.event = event; - } + promises.push(updateOutput( + outputComponentId, + outputProp, + event, + getState, + requestUid, + dispatch + )) + } - const {inputs, state} = dependenciesRequest.content.find( - dependency => ( - dependency.output.id === outputComponentId && - dependency.output.property === outputProp - ) - ) - if (inputs.length > 0) { - payload.inputs = inputs.map(inputObject => { - const propLens = lensPath( - concat(paths[inputObject.id], - ['props', inputObject.property] - )); - return { - id: inputObject.id, - property: inputObject.property, - value: view(propLens, layout) - }; - }); - } - if (state.length > 0) { - payload.state = state.map(stateObject => { - const propLens = lensPath( - concat(paths[stateObject.id], - ['props', stateObject.property] - )); - return { - id: stateObject.id, - property: stateObject.property, - value: view(propLens, layout) - }; - }); - } + return Promise.all(promises); + } +} - promises.push(fetch(`${urlBase(config)}_dash-update-component`, { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - 'X-CSRFToken': cookie.parse(document.cookie)._csrf_token - }, - credentials: 'same-origin', - body: JSON.stringify(payload) - }).then(function handleResponse(res) { - - const getThisRequestIndex = () => { - const postRequestQueue = getState().requestQueue; - const requestUid = newRequestQueue[i].uid; - const thisRequestIndex = findIndex( - propEq('uid', requestUid), - postRequestQueue - ); - return thisRequestIndex; - } +function updateOutput( + outputComponentId, + outputProp, + event, + getState, + requestUid, + dispatch +) { + const { + config, + layout, + graphs, + paths, + dependenciesRequest + } = getState(); + const {InputGraph} = graphs; - const updateRequestQueue = rejected => { - const postRequestQueue = getState().requestQueue - const thisRequestIndex = getThisRequestIndex(); - if (thisRequestIndex === -1) { - // It was already pruned away - return; - } - const updatedQueue = adjust( - merge(__, { - status: res.status, - responseTime: Date.now(), - rejected - }), - thisRequestIndex, - postRequestQueue - ); - // We don't need to store any requests before this one - const thisControllerId = postRequestQueue[ - thisRequestIndex].controllerId; - const prunedQueue = updatedQueue.filter( - (queueItem, index) => { - return ( - queueItem.controllerId !== thisControllerId || - index >= thisRequestIndex - ); - } - ); + /* + * Construct a payload of the input, state, and event. + * For example: + * If the input triggered this update, then: + * { + * inputs: [{'id': 'input1', 'property': 'new value'}], + * state: [{'id': 'state1', 'property': 'existing value'}] + * } + * + * If an event triggered this udpate, then: + * { + * state: [{'id': 'state1', 'property': 'existing value'}], + * event: {'id': 'graph', 'event': 'click'} + * } + * + */ + const payload = { + output: {id: outputComponentId, property: outputProp} + }; - dispatch(setRequestQueue(prunedQueue)); - } + if (event) { + payload.event = event; + } - const isRejected = () => { - const latestRequestIndex = findLastIndex( - propEq('controllerId', newRequestQueue[i].controllerId), - getState().requestQueue + const {inputs, state} = dependenciesRequest.content.find( + dependency => ( + dependency.output.id === outputComponentId && + dependency.output.property === outputProp + ) + ); + if (inputs.length > 0) { + payload.inputs = inputs.map(inputObject => { + const propLens = lensPath( + concat(paths[inputObject.id], + ['props', inputObject.property] + )); + return { + id: inputObject.id, + property: inputObject.property, + value: view(propLens, layout) + }; + }); + } + if (state.length > 0) { + payload.state = state.map(stateObject => { + const propLens = lensPath( + concat(paths[stateObject.id], + ['props', stateObject.property] + )); + return { + id: stateObject.id, + property: stateObject.property, + value: view(propLens, layout) + }; + }); + } + + return fetch(`${urlBase(config)}_dash-update-component`, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'X-CSRFToken': cookie.parse(document.cookie)._csrf_token + }, + credentials: 'same-origin', + body: JSON.stringify(payload) + }).then(function handleResponse(res) { + + const getThisRequestIndex = () => { + const postRequestQueue = getState().requestQueue; + const thisRequestIndex = findIndex( + propEq('uid', requestUid), + postRequestQueue + ); + return thisRequestIndex; + } + + const updateRequestQueue = rejected => { + const postRequestQueue = getState().requestQueue + const thisRequestIndex = getThisRequestIndex(); + if (thisRequestIndex === -1) { + // It was already pruned away + return; + } + const updatedQueue = adjust( + merge(__, { + status: res.status, + responseTime: Date.now(), + rejected + }), + thisRequestIndex, + postRequestQueue + ); + // We don't need to store any requests before this one + const thisControllerId = postRequestQueue[ + thisRequestIndex].controllerId; + const prunedQueue = updatedQueue.filter( + (queueItem, index) => { + return ( + queueItem.controllerId !== thisControllerId || + index >= thisRequestIndex ); - /* - * Note that if the latest request is still `loading` - * or even if the latest request failed, - * we still reject this response in favor of waiting - * for the latest request to finish. - */ - const rejected = latestRequestIndex > getThisRequestIndex(); - return rejected; } + ); - if (res.status !== 200) { - // update the status of this request - updateRequestQueue(true); - return; - } + dispatch(setRequestQueue(prunedQueue)); + } + + const isRejected = () => { + const latestRequestIndex = findLastIndex( + propEq('controllerId', `${outputComponentId}.${outputProp}`), // newRequestQueue[i].controllerId), + getState().requestQueue + ); + /* + * Note that if the latest request is still `loading` + * or even if the latest request failed, + * we still reject this response in favor of waiting + * for the latest request to finish. + */ + 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. + * for example, the could still be request running when + * the user switched the chapter + * + * if it's not visible, then ignore the rest of the updates + * to the store + */ + if (!has(outputComponentId, getState().paths)) { + return; + } + + // and update the props of the component + const observerUpdatePayload = { + itempath: getState().paths[outputComponentId], + // new prop from the server + props: data.response.props, + source: 'response' + }; + dispatch(updateProps(observerUpdatePayload)); + + dispatch(notifyObservers({ + id: outputComponentId, + props: data.response.props + })); + + /* + * If the response includes children, then we need to update our + * paths store. + * TODO - Do we need to wait for updateProps to finish? + */ + if (has('children', observerUpdatePayload.props)) { + + dispatch(computePaths({ + subTree: observerUpdatePayload.props.children, + startingPath: concat( + getState().paths[outputComponentId], + ['props', 'children'] + ) + })); /* - * Check to see if another request has already come back - * _after_ this one. - * If so, ignore this request. + * if children contains objects with IDs, then we + * need to dispatch a propChange for all of these + * new children components */ - if (isRejected()) { - updateRequestQueue(true); - return; - } - - return res.json().then(function handleJson(data) { + if (contains( + type(observerUpdatePayload.props.children), + ['Array', 'Object'] + ) && !isEmpty(observerUpdatePayload.props.children) + ) { /* - * 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 + * TODO: We're just naively crawling + * the _entire_ layout to recompute the + * the dependency graphs. + * We don't need to do this - just need + * to compute the subtree */ - if (isRejected()) { - updateRequestQueue(true); - return; - } - - updateRequestQueue(false); + const newProps = {}; + crawlLayout( + observerUpdatePayload.props.children, + function appendIds(child) { + if (hasId(child)) { + keys(child.props).forEach(childProp => { + const componentIdAndProp = ( + `${child.props.id}.${childProp}` + ); + if (has(componentIdAndProp, InputGraph.nodes)) { + newProps[componentIdAndProp] = ({ + id: child.props.id, + props: { + [childProp]: child.props[childProp] + } + }); + } + }) + } + } + ); /* - * it's possible that this output item is no longer visible. - * for example, the could still be request running when - * the user switched the chapter + * Organize props by shared outputs so that we + * only make one request per output component + * (even if there are multiple inputs). * - * if it's not visible, then ignore the rest of the updates - * to the store + * For example, we might render 10 inputs that control + * a single output. If that is the case, we only want + * to make a single call, not 10 calls. */ - if (!has(outputComponentId, getState().paths)) { - return; - } - - // and update the props of the component - const observerUpdatePayload = { - itempath: getState().paths[outputComponentId], - // new prop from the server - props: data.response.props, - source: 'response' - }; - dispatch(updateProps(observerUpdatePayload)); - - dispatch(notifyObservers({ - id: outputComponentId, - props: data.response.props - })); /* - * If the response includes children, then we need to update our - * paths store. - * TODO - Do we need to wait for updateProps to finish? + * In some cases, the new item will be an output + * with its inputs already rendered (not rendered) + * as part of this update. + * For example, a tab with global controls that + * renders different content containers without any + * additional inputs. + * + * In that case, we'll call `updateOutput` with that output + * and just "pretend" that one if its inputs changed. + * + * If we ever add logic that informs the user on + * "which input changed", we'll have to account for this + * special case (no input changed?) */ - if (has('children', observerUpdatePayload.props)) { - - dispatch(computePaths({ - subTree: observerUpdatePayload.props.children, - startingPath: concat( - getState().paths[outputComponentId], - ['props', 'children'] - ) - })); - - /* - * if children contains objects with IDs, then we - * need to dispatch a propChange for all of these - * new children components - */ - if (contains( - type(observerUpdatePayload.props.children), - ['Array', 'Object'] - ) && !isEmpty(observerUpdatePayload.props.children) - ) { - /* - * TODO: We're just naively crawling - * the _entire_ layout to recompute the - * the dependency graphs. - * We don't need to do this - just need - * to compute the subtree - */ - const newProps = {}; - crawlLayout( - observerUpdatePayload.props.children, - function appendIds(child) { - if (hasId(child)) { - keys(child.props).forEach(childProp => { - const inputId = ( - `${child.props.id}.${childProp}` - ); - if (has(inputId, InputGraph.nodes)) { - newProps[inputId] = ({ - id: child.props.id, - props: { - [childProp]: child.props[childProp] - } - }); - } - }) - } - } - ); + + const outputIds = []; + keys(newProps).forEach(idAndProp => { + if ( + // It's an output + InputGraph.dependenciesOf(idAndProp).length === 0 && /* - * Organize props by shared outputs so that we - * only make one request per output component - * (even if there are multiple inputs). + * And none of its inputs are generated in this + * request */ - const reducedNodeIds = reduceInputIds( - keys(newProps), InputGraph); - const depOrder = InputGraph.overallOrder(); - const sortedNewProps = sort((a, b) => - depOrder.indexOf(a) - depOrder.indexOf(b), - reducedNodeIds - ); - sortedNewProps.forEach(function(nodeId) { - dispatch(notifyObservers(newProps[nodeId])); - }); - + intersection( + InputGraph.dependantsOf(idAndProp), + keys(newProps) + ).length == 0 + ) { + outputIds.push(idAndProp); + delete newProps[idAndProp]; } + }); + + // Dispatch updates to inputs + const reducedNodeIds = reduceInputIds( + keys(newProps), InputGraph); + const depOrder = InputGraph.overallOrder(); + const sortedNewProps = sort((a, b) => + depOrder.indexOf(a.input) - depOrder.indexOf(b.input), + reducedNodeIds + ); + sortedNewProps.forEach(function(inputOutput) { + const payload = newProps[inputOutput.input]; + payload.excludedOutputs = inputOutput.excludedOutputs; + dispatch(notifyObservers(payload)); + }); + + // Dispatch updates to lone outputs + outputIds.forEach(idAndProp => { + const requestUid = uid(); + dispatch(setRequestQueue( + append({ + // TODO - Are there any implications of doing this?? + controllerId: null, + status: 'loading', + uid: requestUid, + requestTime: Date.now() + }, getState().requestQueue) + )); + updateOutput( + idAndProp.split('.')[0], + idAndProp.split('.')[1], + null, + getState, + requestUid, + dispatch + ); + }) + } - } - }); - })); + } - } + }); + }); - return Promise.all(promises); - } } export function serialize(state) { diff --git a/tests/test_render.py b/tests/test_render.py index fc6fb7d..7f3afff 100644 --- a/tests/test_render.py +++ b/tests/test_render.py @@ -4,7 +4,7 @@ import dash_html_components as html import dash_core_components as dcc from .IntegrationTests import IntegrationTests -from .utils import assert_clean_console, invincible, wait_for +from .utils import assert_clean_console, wait_for from multiprocessing import Value import time import re @@ -14,12 +14,34 @@ class Tests(IntegrationTests): def setUp(self): - def wait_for_element_by_id(id): - wait_for(lambda: None is not invincible( - lambda: self.driver.find_element_by_id(id) - )) - return self.driver.find_element_by_id(id) - self.wait_for_element_by_id = wait_for_element_by_id + pass + + def wait_for_element_by_css_selector(self, selector): + start_time = time.time() + exception = Exception('Time ran out, {} not found'.format(selector)) + while time.time() < start_time + 20: + try: + return self.driver.find_element_by_css_selector(selector) + except Exception as e: + exception = e + pass + time.sleep(0.25) + raise exception + + def wait_for_text_to_equal(self, selector, assertion_text): + start_time = time.time() + exception = Exception('Time ran out, {} on {} not found'.format( + assertion_text, selector)) + while time.time() < start_time + 20: + el = self.wait_for_element_by_css_selector(selector) + try: + return self.assertEqual(el.text, assertion_text) + except Exception as e: + exception = e + pass + time.sleep(0.25) + + raise exception def request_queue_assertions( self, check_rejected=True, expected_length=None): @@ -85,7 +107,7 @@ def test_initial_state(self): self.startServer(app) - el = self.wait_for_element_by_id('_dash-app-content') + el = self.wait_for_element_by_css_selector('#_dash-app-content') # TODO - Make less fragile with http://lxml.de/lxmlhtml.html#html-diff rendered_dom = ''' @@ -440,17 +462,15 @@ def update_output(value): self.startServer(app) - output1 = self.wait_for_element_by_id('output-1') - wait_for(lambda: output1.text == 'initial value') + self.wait_for_text_to_equal('#output-1', 'initial value') self.percy_snapshot(name='simple-callback-1') - input1 = self.wait_for_element_by_id('input') + input1 = self.wait_for_element_by_css_selector('#input') input1.clear() input1.send_keys('hello world') - output1 = lambda: self.wait_for_element_by_id('output-1') - wait_for(lambda: output1().text == 'hello world') + self.wait_for_text_to_equal('#output-1', 'hello world') self.percy_snapshot(name='simple-callback-2') self.assertEqual( @@ -517,13 +537,19 @@ def update_input(value): wait_for( lambda: ( self.driver.find_element_by_id('output') - .get_attribute('innerHTML') == ''' -