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.

Sign up for GitHub

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

setProps and rendered event ordering causing unsent callbacks #1415

Merged
merged 7 commits into from
Sep 24, 2020
Merged

setProps and rendered event ordering causing unsent callbacks #1415

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
174252a
Dispatch props update after notifying observers
Sep 23, 2020
53f0bb2
Add test for callback behavior on delayed `setProps`
Sep 23, 2020
958ead3
changelog
Sep 23, 2020
73304f8
Update @plotly/dash-test-components/src/fragments/AsyncComponentLoade…
Marc-Andre-Rivet Sep 24, 2020
6a65d2c
trigger build
Sep 24, 2020
f20be46
Merge branch 'event-ordering' of github.com:plotly/dash into event-or…
Sep 24, 2020
d3dd668
CollapseComponent - provides a close equivalent to dcc.Tabs (children…
Sep 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion @plotly/dash-test-components/babel.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,8 @@ const presets = [
'@babel/preset-react'
];

module.exports = { presets };
const plugins = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add async support for the test components

'@babel/plugin-syntax-dynamic-import'
];

module.exports = { presets, plugins };
12 changes: 12 additions & 0 deletions @plotly/dash-test-components/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions @plotly/dash-test-components/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,11 @@
"devDependencies": {
"@babel/cli": "^7.4.0",
"@babel/core": "^7.4.0",
"@babel/plugin-syntax-dynamic-import": "^7.8.3",
"@babel/preset-env": "^7.4.1",
"@babel/preset-react": "^7.0.0",
"@plotly/dash-component-plugins": "^1.2.0",
"@plotly/webpack-dash-dynamic-import": "^1.1.5",
"babel-loader": "^8.0.5",
"npm-run-all": "^4.1.5",
"react": "16.13.0",
Expand Down
19 changes: 19 additions & 0 deletions @plotly/dash-test-components/src/components/AsyncComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import PropTypes from 'prop-types';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a set of components with simple behaviors corresponding to various usage scenarios. This component is async.

import React, { Suspense } from 'react';
import { asyncDecorator } from '@plotly/dash-component-plugins';
import asyncComponentLoader from './../fragments/AsyncComponentLoader';

const AsyncComponent = props => (<Suspense fallback={null}>
<RealAsyncComponent {...props} />
</Suspense>);

const RealAsyncComponent = asyncDecorator(AsyncComponent, asyncComponentLoader);

AsyncComponent.propTypes = {
id: PropTypes.string,
value: PropTypes.string
};

AsyncComponent.defaultProps = {};

export default AsyncComponent;
18 changes: 18 additions & 0 deletions @plotly/dash-test-components/src/components/DelayedEventComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import PropTypes from 'prop-types';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a set of components with simple behaviors corresponding to various usage scenarios. This component delays setProps until later.

import React from 'react';

const DelayedEventComponent = ({ id, n_clicks, setProps }) => (<button
id={id}
onClick={() => setTimeout(() => setProps({ n_clicks: n_clicks + 1 }), 20)}
/>);

DelayedEventComponent.propTypes = {
id: PropTypes.string,
n_clicks: PropTypes.number
};

DelayedEventComponent.defaultProps = {
n_clicks: 0
};

export default DelayedEventComponent;
15 changes: 15 additions & 0 deletions @plotly/dash-test-components/src/components/FragmentComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import PropTypes from 'prop-types';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a set of components with simple behaviors corresponding to various usage scenarios. This is a top-level component without n_clicks

import React, { Fragment } from 'react';

const FragmentComponent = props => (<Fragment>
{props.children}
</Fragment>);

FragmentComponent.propTypes = {
children: PropTypes.node,
id: PropTypes.string
};

FragmentComponent.defaultProps = {};

export default FragmentComponent;
19 changes: 19 additions & 0 deletions @plotly/dash-test-components/src/fragments/AsyncComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import PropTypes from 'prop-types';
import React, { Fragment } from 'react';
import { asyncDecorator } from '@plotly/dash-component-plugins';

/**
* MyComponent description
*/
const AsyncComponent = ({ value }) => (<Fragment>
{value}
</Fragment>);

AsyncComponent.propTypes = {
id: PropTypes.string,
value: PropTypes.string
};

AsyncComponent.defaultProps = {};

export default AsyncComponent;
1 change: 1 addition & 0 deletions @plotly/dash-test-components/src/fragments/AsyncComponentLoader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default () => import('./../fragments/AsyncComponent');
10 changes: 9 additions & 1 deletion @plotly/dash-test-components/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,16 @@
import AsyncComponent from './components/AsyncComponent';
import DelayedEventComponent from './components/DelayedEventComponent';
import FragmentComponent from './components/FragmentComponent';
import StyledComponent from './components/StyledComponent';
import MyPersistedComponent from './components/MyPersistedComponent';
import MyPersistedComponentNested from './components/MyPersistedComponentNested';


export {
StyledComponent, MyPersistedComponent, MyPersistedComponentNested
AsyncComponent,
DelayedEventComponent,
FragmentComponent,
MyPersistedComponent,
MyPersistedComponentNested,
StyledComponent
};
22 changes: 21 additions & 1 deletion @plotly/dash-test-components/webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const path = require('path');
const webpack = require('webpack');
const WebpackDashDynamicImport = require('@plotly/webpack-dash-dynamic-import');

const packagejson = require('./package.json');

Expand All @@ -14,6 +15,7 @@ module.exports = {
},
output: {
path: path.resolve(__dirname, dashLibraryName),
chunkFilename: '[name].js',
filename: `${dashLibraryName}.js`,
library: dashLibraryName,
libraryTarget: 'window',
Expand All @@ -28,5 +30,23 @@ module.exports = {
}
}
],
}
},
optimization: {
splitChunks: {
chunks: 'async',
name: true,
cacheGroups: {
async: {
chunks: 'async',
minSize: 0,
name(module, chunks, cacheGroupKey) {
return `${cacheGroupKey}-${chunks[0].name}`;
}
}
}
}
},
plugins: [
new WebpackDashDynamicImport()
]
};
16 changes: 8 additions & 8 deletions dash-renderer/src/TreeContainer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,14 +138,6 @@ class BaseTreeContainer extends Component {
// for persistence
recordUiEdit(_dashprivate_layout, newProps, _dashprivate_dispatch);

// Always update this component's props
_dashprivate_dispatch(
updateProps({
props: changedProps,
itempath: _dashprivate_path
})
);

// Only dispatch changes to Dash if a watched prop changed
if (watchedKeys.length) {
_dashprivate_dispatch(
Expand All @@ -155,6 +147,14 @@ class BaseTreeContainer extends Component {
})
);
}

// Always update this component's props
_dashprivate_dispatch(
updateProps({
props: changedProps,
itempath: _dashprivate_path
})
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Callbacks are processed after a 0ms timeout - updating the props after triggering the notifications ensures the notifications are ready on re-render.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to make sure I'm understanding this correctly: notifyObservers is what ultimately triggers the callback processing and attaches the rendered event listener. This is async (with a zero delay), but so is the rerender caused by updateProps, and by reversing the order here we ensure that the event listener is inserted in the queue before the rerender so we're in the clear.

Do I have that right?

It still feels a bit fragile, like what we ideally would want to do is attach the event listener synchronously here somehow and just let it be used later, in case another 0ms delay got added unexpectedly. But that might be invasive, and anyway you've got a very clear test - very nice collection of new test components!

Copy link
Contributor Author

@Marc-Andre-Rivet Marc-Andre-Rivet Sep 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes about the ordering change. TBH I have the same discomfort about the fragility or at least the fragmentation of the implementation between the APIController useEffect, https://github.com/plotly/dash/blob/dev/dash-renderer/src/actions/isAppReady.js#L38 DOM check and the async wait in observers/requestedCallbacks. What's certain is that we want the event to be triggered when we know the DOM has been updated after a render, otherwise the DOM check might be moot, and that pretty much leaves only useEffect/componentDidUpdate/componentDidMount.

I've tested adding an additional 0ms async delay prior to calling notifyObservers and it does fail the test, so a change in timing would probably be caught, which is comforting..

}
}

Expand Down
56 changes: 55 additions & 1 deletion tests/integration/callbacks/test_basic_callback.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
import json
from multiprocessing import Lock, Value

import pytest

import dash_core_components as dcc
import dash_html_components as html
import dash_table
import dash
from dash_test_components import (
AsyncComponent,
DelayedEventComponent,
FragmentComponent,
)
from dash.dependencies import Input, Output, State
from dash.exceptions import PreventUpdate
from dash.testing import wait
Expand Down Expand Up @@ -404,3 +408,53 @@ def update_text(data):
assert input_call_count.value == 2 + len("hello world")

assert not dash_duo.get_logs()


def test_cbsc009_callback_using_unloaded_async_component_and_graph(dash_duo):
app = dash.Dash(__name__)
app.layout = FragmentComponent(
children=[
dcc.Tabs(
value="1",
children=[
dcc.Tab(label="Tab 1", value="1", children="Tab 1 Content"),
dcc.Tab(
label="2",
value="2",
children=AsyncComponent(id="async", value="A"),
),
],
),
html.Button("n", id="n"),
DelayedEventComponent(id="d"),
html.Div("Output init", id="output"),
]
)

@app.callback(
Output("output", "children"),
Input("n", "n_clicks"),
Input("d", "n_clicks"),
Input("async", "value"),
)
def content(n, d, v):
return json.dumps([n, d, v])

dash_duo.start_server(app)

wait.until(lambda: dash_duo.find_element("#output").text == '[null, null, "A"]', 3)
dash_duo.wait_for_element("#d").click()

wait.until(
lambda: dash_duo.find_element("#output").text == '[null, 1, "A"]', 3,
)

dash_duo.wait_for_element("#n").click()
wait.until(
lambda: dash_duo.find_element("#output").text == '[1, 1, "A"]', 3,
)

dash_duo.wait_for_element("#d").click()
wait.until(
lambda: dash_duo.find_element("#output").text == '[1, 2, "A"]', 3,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simplified version of the original reproducible example involving the DataTable inside a dcc.Tab and the dcc.Graph, all inside a non-click reactive top-level component -- does not do away with all real components as it would be tedious but used custom made components for the key behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was run and fails against dash-renderer==1.8.1

)