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

setProps and rendered event ordering causing unsent callbacks #1415

Merged
merged 7 commits into from
Sep 24, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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

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
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
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;
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 hides its content by default (simplified version of what dcc.Tabs/dcc.Tab do)

import React, { Fragment } from 'react';

const CollapseComponent = props => (<Fragment>
{display ? props.children : null}
</Fragment>);

CollapseComponent.propTypes = {
children: PropTypes.node,
display: PropTypes.bool,
id: PropTypes.string
};

CollapseComponent.defaultProps = {
display: false
};

export default CollapseComponent;
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;
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
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;
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default () => import('./AsyncComponent');
12 changes: 11 additions & 1 deletion @plotly/dash-test-components/src/index.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,18 @@
import AsyncComponent from './components/AsyncComponent';
import CollapseComponent from './components/CollapseComponent';
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,
CollapseComponent,
DelayedEventComponent,
FragmentComponent,
MyPersistedComponent,
MyPersistedComponentNested,
StyledComponent
};
22 changes: 21 additions & 1 deletion @plotly/dash-test-components/webpack.config.js
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()
]
};
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@
All notable changes to `dash` will be documented in this file.
This project adheres to [Semantic Versioning](https://semver.org/).

## Unreleased
### Fixed
- [#1415](https://github.com/plotly/dash/pull/1415) Fix a regression with some layouts callbacks involving dcc.Tabs, not yet loaded dash_table.DataTable and dcc.Graph to not be called

## [1.16.1] - 2020-09-16
### Changed
- [#1376](https://github.com/plotly/dash/pull/1376) Extends the `getTransform` logic in the renderer to handle `persistenceTransforms` for both nested and non-nested persisted props. This was used to to fix [dcc#700](https://github.com/plotly/dash-core-components/issues/700) in conjunction with [dcc#854](https://github.com/plotly/dash-core-components/pull/854) by using persistenceTransforms to strip the time part of the datetime so that datepickers can persist when defined in callbacks.
Expand Down
16 changes: 8 additions & 8 deletions dash-renderer/src/TreeContainer.js
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

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
47 changes: 46 additions & 1 deletion tests/integration/callbacks/test_basic_callback.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
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,
CollapseComponent,
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 +409,43 @@ 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(
[
CollapseComponent([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

)