Issue 1010 - Non-rendered async components using asyncDecorator lock renderer callbacks
#1027
Conversation
| return promises.length ? Promise.all(promises) : true; | ||
| return promises.length ? Promise.all(promises) : true; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Change isAppReady to return either the global state of the app or the state for the target ids given.
| if n_clicks is None: | ||
| raise PreventUpdate | ||
|
|
||
| return "Bye" |
There was a problem hiding this comment.
Before the changes in this PR, this callback is never triggered because of the blocking dash-table (uses the asyncDecorator) in the other tab. Now, since the callback only involves btn and output, the table is non-blocking.
Marc-Andre-Rivet
changed the title
isReady decorator prevents callbacks from executingisReady decorator prevents callbacks from executing
Marc-Andre-Rivet
requested review from
alexcjohnson and
byronz
as code owners
Marc-Andre-Rivet
changed the title
isReady decorator prevents callbacks from executingasyncDecorator lock renderer callbacks
| components[namespace][type] = type; | ||
| const isAppReady = (layout, paths) => targets => { | ||
| if (!layout || !paths || !Array.isArray(targets)) { | ||
| return true; |
There was a problem hiding this comment.
Is there a use case for this? Feels like an error rather than a silent success.
There was a problem hiding this comment.
Relates to the comment below. Refactoring to take isAppReady outside of the state, timing issues will be moot.
|
|
||
| components[namespace] = components[namespace] || {}; | ||
| components[namespace][type] = type; | ||
| const isAppReady = (layout, paths) => targets => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ie to just make it a function (layout, paths, targets) => {...}
There was a problem hiding this comment.
exactly the problem making the tests fail, the store evals IS_APP_READY before COMPUTE_PATH and the first calls ends up not blocking
|
|
||
| graph_2_expected_clickdata = { | ||
| "points": [{"curveNumber": 0, "pointNumber": 1, "pointIndex": 1, "x": 3, "y": 10}] | ||
| "points": [{"curveNumber": 0, "pointNumber": 1, "pointIndex": 1, "x": 3, "y": 10, "label": 3, "value": 10}] |
There was a problem hiding this comment.
@alexcjohnson Getting the same error you have in #1026 -- there seems to be additional label/value props
alexcjohnson
left a comment
alexcjohnson
left a comment
There was a problem hiding this comment.
It's a beautiful thing when - even with new tests - a bug fix results in the codebase shrinking 🎉 💪 💃
Fixes #1010
It is possible for a component in the layout not to be displayed (e.g. tabs) and as such, async resources associated with it will not be loaded. If the component uses the
asyncDecoratorthis means that the component could block the renderer's callbacks indefinitely.This change reduces the scope of the renderer's callback lock by only using the components required as input/state to determine the required lock.
A new test has been added and demonstrated to fail prior to adding the fix (f5e577e)