Skip to content

Issue 1010 - Non-rendered async components using asyncDecorator lock renderer callbacks #1027

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

Merged
merged 14 commits into from
Nov 26, 2019
Merged

Issue 1010 - Non-rendered async components using asyncDecorator lock renderer callbacks #1027

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
f5e577e
failing callback test with not-loaded async component
Nov 22, 2019
6e9f11b
Update isAppReady check to check only involved (input, state) compone…
Nov 22, 2019
2ead779
lint
Nov 22, 2019
11b8877
fix notifyObserver test
Nov 22, 2019
340aa26
changelog
Nov 23, 2019
324d8d6
simpify isReady by (1) only allowing the targets case, (2) using path…
Nov 26, 2019
bd02bff
format
Nov 26, 2019
50f3e58
refactor isAppReady out of state/store
Nov 26, 2019
0e2c54a
lint
Nov 26, 2019
796db0c
diff testing vs. dcc with plotly.js 1.51.2
Nov 26, 2019
c0aaab2
plotly.js 1.51.2
Nov 26, 2019
73ce153
update test json
Nov 26, 2019
7a68d13
uncomment!!
Nov 26, 2019
39a6915
rename test file to match usage
Nov 26, 2019
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
92 changes: 24 additions & 68 deletions dash-renderer/src/actions/setAppReadyState.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,87 +1,43 @@
import {filter} from 'ramda';
import {createAction} from 'redux-actions';
import { path } from 'ramda';
import { createAction } from 'redux-actions';

import isSimpleComponent from '../isSimpleComponent';
import Registry from './../registry';
import {getAction} from './constants';
import {isReady} from '@plotly/dash-component-plugins';
import { getAction } from './constants';
import { isReady } from '@plotly/dash-component-plugins';

const isAppReady = layout => {
const queue = [layout];

const components = {};
const ids = {};

/* Would be much simpler if the Registry was aware of what it contained... */
while (queue.length) {
const elementLayout = queue.shift();
if (!elementLayout) {
continue;
}

const id = elementLayout.props && elementLayout.props.id;
const children = elementLayout.props && elementLayout.props.children;
const namespace = elementLayout.namespace;
const type = elementLayout.type;

components[namespace] = components[namespace] || {};
components[namespace][type] = type;
const isAppReady = (layout, paths) => targets => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

As long as we get the order of operations right (ie always updating isAppReady after any update to layout or paths) this should be fine, but it would be more robust and simpler - without any cost anymore - to not put this in the state at all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ie to just make it a function (layout, paths, targets) => {...}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exactly the problem making the tests fail, the store evals IS_APP_READY before COMPUTE_PATH and the first calls ends up not blocking

if (!layout || !paths || !Array.isArray(targets)) {
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a use case for this? Feels like an error rather than a silent success.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Relates to the comment below. Refactoring to take isAppReady outside of the state, timing issues will be moot.

}

if (id) {
ids[id] = {namespace, type};
const promises = [];
targets.forEach(id => {
const pathOfId = paths[id];
if (!pathOfId) {
return;
}

if (children) {
const filteredChildren = filter(
child => !isSimpleComponent(child),
Array.isArray(children) ? children : [children]
);

queue.push(...filteredChildren);
const target = path(pathOfId, layout);
if (!target) {
return;
}
}

return targets => {
const promises = [];

if (Array.isArray(targets)) {
targets.forEach(id => {
const target = ids[id];
if (target) {
const component = Registry.resolve(target);

const ready = isReady(component);

if (ready && typeof ready.then === 'function') {
promises.push(ready);
}
}
});
} else {
Object.entries(components).forEach(([namespace, item]) => {
Object.entries(item).forEach(([type]) => {
const component = Registry.resolve({
namespace,
type,
});

const ready = isReady(component);
const component = Registry.resolve(target);
const ready = isReady(component);

if (ready && typeof ready.then === 'function') {
promises.push(ready);
}
});
});
if (ready && typeof ready.then === 'function') {
promises.push(ready);
}
});

return promises.length ? Promise.all(promises) : true;
};
return promises.length ? Promise.all(promises) : true;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change isAppReady to return either the global state of the app or the state for the target ids given.


const setAction = createAction(getAction('SET_APP_READY'));

export default () => async (dispatch, getState) => {
const ready = isAppReady(getState().layout);
const { layout, paths } = getState();
const ready = isAppReady(layout, paths);

dispatch(setAction(ready));
};