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

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

Merged
merged 28 commits into from
Sep 3, 2020
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
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.

54 changes: 54 additions & 0 deletions dash-renderer/src/observers/documentTitle.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
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));
observer.mutationObserver.observe(
document.querySelector('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