Skip to content

Issue 1350 - Multi-input callback with sync event handling #1385

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

Merged
merged 28 commits into from
Sep 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
a8cc367
- delay `requestedCallbacks` processing so that callbacks triggered b…
Aug 27, 2020
074e58a
noise
Aug 27, 2020
783953b
wait for render event
Aug 28, 2020
60e8508
multiple output -> tuple or list
Aug 28, 2020
a5cb50d
DocumentTitle observer instead of empty component
Aug 28, 2020
f795fdb
config update_title
Aug 28, 2020
8b6ff26
- don't group initial callbacks with subsequent callbacks
Aug 28, 2020
588afa0
always drop initialCallback if there's duplicates
Aug 31, 2020
d32e88f
last exgroup
Aug 31, 2020
d52cee7
trigger build
Aug 31, 2020
f9e9bdb
test clickthrough / callback behavior
Aug 31, 2020
0dd6ce1
changelog
Aug 31, 2020
56cac61
lint
Aug 31, 2020
2b8a2e3
simplify requestedCallbacks update
Sep 1, 2020
8471e14
context initial callbacks test
Sep 1, 2020
f4086c6
test initial callback + user action callbacks
Sep 1, 2020
3c0b127
Merge branch 'dev' into 1350-callback-regression
Marc-Andre-Rivet Sep 1, 2020
90697b1
no global variables
Sep 2, 2020
d057cd7
Merge branch '1350-callback-regression' of github.com:plotly/dash int…
Sep 2, 2020
6fe5040
Merge branch 'dev' into 1350-callback-regression
Marc-Andre-Rivet Sep 2, 2020
ed20242
trigger build
Sep 2, 2020
5988554
Merge branch '1350-callback-regression' of github.com:plotly/dash int…
Sep 2, 2020
ae33983
trigger build
Sep 2, 2020
9c87c66
check document.title element exists
Sep 2, 2020
49fb65e
improve cbsc
Sep 2, 2020
eca7ed1
wait
Sep 2, 2020
4588463
trigger build
Sep 2, 2020
cd1abbf
more locks
Sep 2, 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
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ All notable changes to `dash` will be documented in this file.
This project adheres to [Semantic Versioning](https://semver.org/).

## [UNRELEASED]
### Changed
- [#1385](https://github.com/plotly/dash/pull/1385) Closes [#1350](https://github.com/plotly/dash/issues/1350) and fixes a previously undefined callback behavior when multiple elements are stacked on top of one another and their `n_clicks` props are used as inputs of the same callback. The callback will now trigger once with all the triggered `n_clicks` props changes.

### Fixed
- [#1384](https://github.com/plotly/dash/pull/1384) Fixed a bug introduced by [#1180](https://github.com/plotly/dash/pull/1180) breaking use of `prevent_initial_call` as a positional arg in callback definitions

Expand Down
8 changes: 6 additions & 2 deletions dash-renderer/src/APIController.react.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {applyPersistence} from './persistence';
import {getAppState} from './reducers/constants';
import {STATUS} from './constants/constants';
import {getLoadingState, getLoadingHash} from './utils/TreeContainer';
import wait from './utils/wait';

export const DashContext = createContext({});

Expand Down Expand Up @@ -63,8 +64,11 @@ const UnconnectedContainer = props => {

useEffect(() => {
if (renderedTree.current) {
renderedTree.current = false;
events.current.emit('rendered');
(async () => {
renderedTree.current = false;
await wait(0);
Copy link
Contributor Author

@Marc-Andre-Rivet Marc-Andre-Rivet Aug 31, 2020

Choose a reason for hiding this comment

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

I really dislike this artificial delay being required to counteract the additional processing delay in https://github.com/plotly/dash/pull/1385/files#diff-178b2bd05a773877fd1b117eef8b1e8cR63 but I can't think of a better alternative atm.

events.current.emit('rendered');
})();
}
});

Expand Down
2 changes: 0 additions & 2 deletions dash-renderer/src/AppContainer.react.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import {connect} from 'react-redux';
import React from 'react';
import PropTypes from 'prop-types';
import APIController from './APIController.react';
import DocumentTitle from './components/core/DocumentTitle.react';
import Loading from './components/core/Loading.react';
import Toolbar from './components/core/Toolbar.react';
import Reloader from './components/core/Reloader.react';
Expand Down Expand Up @@ -48,7 +47,6 @@ class UnconnectedAppContainer extends React.Component {
<React.Fragment>
{show_undo_redo ? <Toolbar /> : null}
<APIController />
<DocumentTitle />
<Loading />
<Reloader />
</React.Fragment>
Expand Down
1 change: 1 addition & 0 deletions dash-renderer/src/StoreObserver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ interface IStoreObserverState<TStore> {
export interface IStoreObserverDefinition<TStore> {
observer: Observer<Store<TStore>>;
inputs: string[]
[key: string]: any;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just some flexibility when creating observers

}

export default class StoreObserver<TStore> {
Expand Down
51 changes: 0 additions & 51 deletions dash-renderer/src/components/core/DocumentTitle.react.js

This file was deleted.

58 changes: 58 additions & 0 deletions dash-renderer/src/observers/documentTitle.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import { IStoreObserverDefinition } from '../StoreObserver';
import { IStoreState } from '../store';

const updateTitle = (getState: () => IStoreState) => {
const {
config,
isLoading
} = getState();

const update_title = config?.update_title;

if (!update_title) {
return;
}

if (isLoading) {
if (document.title !== update_title) {
observer.title = document.title;
document.title = update_title;
}
} else {
if (document.title === update_title) {
document.title = observer.title;
} else {
observer.title = document.title;
}
}
};

const observer: IStoreObserverDefinition<IStoreState> = {
inputs: ['isLoading'],
mutationObserver: undefined,
observer: ({
getState
}) => {
const {
config
} = getState();

if (observer.config !== config) {
observer.config = config;
observer.mutationObserver?.disconnect();
observer.mutationObserver = new MutationObserver(() => updateTitle(getState));

const title = document.querySelector('title');
if (title) {
observer.mutationObserver.observe(
title,
{ subtree: true, childList: true, attributes: true, characterData: true }
);
}
}

updateTitle(getState);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaces <DocumentTitle /> and makes it arguably simpler to handle store changes and DOM mutations in a consistent manner as it doesn't need to deal with component lifecycle anymore.

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 solution should be more robust as it can react to both the DOM changing and the store changing in ways that are not dependant on exact execution timing (the additional wait broke the test/behavior)

}
};

export default observer;
85 changes: 58 additions & 27 deletions dash-renderer/src/observers/requestedCallbacks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,29 +4,34 @@ import {
difference,
filter,
flatten,
forEach,
groupBy,
includes,
intersection,
isEmpty,
isNil,
map,
mergeLeft,
mergeWith,
pluck,
reduce,
values
} from 'ramda';

import { IStoreState } from '../store';

import {
aggregateCallbacks,
removeRequestedCallbacks,
removePrioritizedCallbacks,
removeExecutingCallbacks,
removeWatchedCallbacks,
addRequestedCallbacks,
addPrioritizedCallbacks,
addExecutingCallbacks,
addWatchedCallbacks,
removeBlockedCallbacks,
addBlockedCallbacks
addBlockedCallbacks,
addRequestedCallbacks,
removeRequestedCallbacks
} from '../actions/callbacks';

import { isMultiValued } from '../actions/dependencies';
Expand All @@ -45,17 +50,23 @@ import {
IBlockedCallback
} from '../types/callbacks';

import wait from './../utils/wait';

import { getPendingCallbacks } from '../utils/callbacks';
import { IStoreObserverDefinition } from '../StoreObserver';

const observer: IStoreObserverDefinition<IStoreState> = {
observer: ({
observer: async ({
dispatch,
getState
}) => {
await wait(0);
Copy link
Contributor Author

@Marc-Andre-Rivet Marc-Andre-Rivet Aug 28, 2020

Choose a reason for hiding this comment

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

Adding this little wait apparently has wide-ranging consequences on the timing of various features. So far:


const { callbacks, callbacks: { prioritized, blocked, executing, watched, stored }, paths } = getState();
let { callbacks: { requested } } = getState();

const initialRequested = requested.slice(0);

const pendingCallbacks = getPendingCallbacks(callbacks);

/*
Expand All @@ -78,17 +89,37 @@ const observer: IStoreObserverDefinition<IStoreState> = {
1. Remove duplicated `requested` callbacks - give precedence to newer callbacks over older ones
*/

/*
Extract all but the first callback from each IOS-key group
these callbacks are duplicates.
*/
const rDuplicates = flatten(map(
group => group.slice(0, -1),
values(
groupBy<ICallback>(
getUniqueIdentifier,
requested
)
let rDuplicates: ICallback[] = [];
let rMergedDuplicates: ICallback[] = [];

forEach(group => {
if (group.length === 1) {
// keep callback if its the only one of its kind
rMergedDuplicates.push(group[0]);
} else {
const initial = group.find(cb => cb.initialCall);
if (initial) {
// drop the initial callback if it's not alone
rDuplicates.push(initial);
}

const groupWithoutInitial = group.filter(cb => cb !== initial);
if (groupWithoutInitial.length === 1) {
// if there's only one callback beside the initial one, keep that callback
rMergedDuplicates.push(groupWithoutInitial[0]);
} else {
// otherwise merge all remaining callbacks together
rDuplicates = concat(rDuplicates, groupWithoutInitial);
rMergedDuplicates.push(mergeLeft({
changedPropIds: reduce(mergeWith(Math.max), {}, pluck('changedPropIds', groupWithoutInitial)),
executionGroup: filter(exg => !!exg, pluck('executionGroup', groupWithoutInitial)).slice(-1)[0]
}, groupWithoutInitial.slice(-1)[0]) as ICallback);
}
}
}, values(
groupBy<ICallback>(
getUniqueIdentifier,
requested
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merge callbacks, keep correct changePropIds DIRECT/INDIRECT and last know executionGroup if there is one. Always drop the initial callback if there is one and there are duplicates.

)
));

Expand All @@ -97,7 +128,7 @@ const observer: IStoreObserverDefinition<IStoreState> = {
Clean up the `requested` list - during the dispatch phase,
duplicates will be removed for real
*/
requested = difference(requested, rDuplicates);
requested = rMergedDuplicates;

/*
2. Remove duplicated `prioritized`, `executing` and `watching` callbacks
Expand Down Expand Up @@ -312,16 +343,24 @@ const observer: IStoreObserverDefinition<IStoreState> = {
dropped
);

requested = difference(
requested,
readyCallbacks
);

const added = difference(requested, initialRequested);
const removed = difference(initialRequested, requested);

dispatch(aggregateCallbacks([
// Clean up requested callbacks
added.length ? addRequestedCallbacks(added) : null,
removed.length ? removeRequestedCallbacks(removed) : null,
// Clean up duplicated callbacks
rDuplicates.length ? removeRequestedCallbacks(rDuplicates) : null,
pDuplicates.length ? removePrioritizedCallbacks(pDuplicates) : null,
bDuplicates.length ? removeBlockedCallbacks(bDuplicates) : null,
eDuplicates.length ? removeExecutingCallbacks(eDuplicates) : null,
wDuplicates.length ? removeWatchedCallbacks(wDuplicates) : null,
// Prune callbacks
rRemoved.length ? removeRequestedCallbacks(rRemoved) : null,
rAdded.length ? addRequestedCallbacks(rAdded) : null,
pRemoved.length ? removePrioritizedCallbacks(pRemoved) : null,
pAdded.length ? addPrioritizedCallbacks(pAdded) : null,
bRemoved.length ? removeBlockedCallbacks(bRemoved) : null,
Expand All @@ -330,15 +369,7 @@ const observer: IStoreObserverDefinition<IStoreState> = {
eAdded.length ? addExecutingCallbacks(eAdded) : null,
wRemoved.length ? removeWatchedCallbacks(wRemoved) : null,
wAdded.length ? addWatchedCallbacks(wAdded) : null,
// Prune circular callbacks
rCirculars.length ? removeRequestedCallbacks(rCirculars) : null,
// Prune circular assumptions
oldBlocked.length ? removeRequestedCallbacks(oldBlocked) : null,
newBlocked.length ? addRequestedCallbacks(newBlocked) : null,
// Drop non-triggered initial callbacks
dropped.length ? removeRequestedCallbacks(dropped) : null,
// Promote callbacks
readyCallbacks.length ? removeRequestedCallbacks(readyCallbacks) : null,
readyCallbacks.length ? addPrioritizedCallbacks(readyCallbacks) : null
]));
},
Expand Down
2 changes: 2 additions & 0 deletions dash-renderer/src/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { ICallbacksState } from './reducers/callbacks';
import { LoadingMapState } from './reducers/loadingMap';
import { IsLoadingState } from './reducers/isLoading';

import documentTitle from './observers/documentTitle';
import executedCallbacks from './observers/executedCallbacks';
import executingCallbacks from './observers/executingCallbacks';
import isLoading from './observers/isLoading'
Expand All @@ -33,6 +34,7 @@ const storeObserver = new StoreObserver<IStoreState>();
const setObservers = once(() => {
const observe = storeObserver.observe;

observe(documentTitle);
observe(isLoading);
observe(loadingMap);
observe(requestedCallbacks);
Expand Down
8 changes: 8 additions & 0 deletions dash-renderer/src/utils/wait.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export default async (duration: number) => {
let _resolve: any;
const p = new Promise(resolve => _resolve = resolve);

setTimeout(_resolve, duration);

return p;
}
Loading