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

Conversation

Marc-Andre-Rivet
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet commented Sep 23, 2020

This PR proposes to fix a timing issue causing callbacks not to be triggered under certain circumstances.

Possibly a regression in 1.16.0 caused by #1385 timing changes.

The issue occurs when a callback depends on:

  1. An unloaded (e.g. inside an unfocused dcc.Tab) async component (e.g. DataTable)
  2. A dcc.Graph - or a component that delays setProps calls / events handling
  3. All components above the DataTable and Graph in the hierarchy are not click reactive (e.g. no n_clicks prop)

Interestingly enough this may not be a (recent) regression, or a regression at all. Running any Dash 1.11+ against test_basic_callbacks.py tests fails on the new cbsc009 test.

Marc-André Rivet added 2 commits September 23, 2020 09:41
- new test components: AsyncComponent, DelayedEventComponent, FragmentComponent
@@ -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.

@@ -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.

@@ -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

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..


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

@@ -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

@@ -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)

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃

@Marc-Andre-Rivet Marc-Andre-Rivet merged commit cea5aba into dev Sep 24, 2020
@Marc-Andre-Rivet Marc-Andre-Rivet deleted the event-ordering branch September 24, 2020 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants