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

Missing inputs #1212

Merged
merged 5 commits into from
Apr 28, 2020
Merged

Missing inputs #1212

merged 5 commits into from
Apr 28, 2020

Conversation

alexcjohnson
Copy link
Collaborator

@alexcjohnson alexcjohnson commented Apr 26, 2020

Fixes #1200 - regression in Dash 1.11 with callbacks that have outputs but no inputs.

  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

  • I have added entry in the CHANGELOG.md
  • If this PR needs a follow-up in dash docs, community thread, I have mentioned the relevant URLS as follow
    • at some point we should add an in-depth exploration of these cases to the docs for callbacks. Maybe not needed right away though, as we're primarily just fixing a regression.
    • we'll follow up in the v1.11 forum post by linking it to the v1.12 post.


def test_cbmi007_all_multi_wildcards_some_outputs(dash_duo):
# same as above (cbmi006) but multi-output, some outputs present some missing.
# Again we DO NOT fire the callback
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a case I didn't think of until writing related tests, but I think it makes sense, because if there's any missing output and no inputs, that output will probably show up when the inputs do.

This is basically the same as test cbmi004 just for the case when all the inputs have multi-value wildcards so we do try to fire the callback, and we need to bail out at a later step when we collect all the outputs.

@alexcjohnson
Copy link
Collaborator Author

The first 4 of these tests (modified for the changed internals) pass when run against Dash v1.10 - so this clears the regression as far as I understand it. The other 5 tests all use wildcards so are not relevant to v1.10. cc @chriddyp @Marc-Andre-Rivet

if (e instanceof ReferenceError) {
errors.push(e);
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering what would be required to be able to write this (and https://github.com/plotly/dash/pull/1212/files#diff-15692ba768eeab56e95aa409ab7c19b7R279) so that we don't use exceptions as flow control.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

don't use exceptions as flow control

yeah I suppose that's cleaner now that they aren't always errors -> b44bd54

// Wildcard reference errors mention a list of wildcard specs logged
// TODO: unwrapped list of wildcard ids?
// eslint-disable-next-line no-console
console.log(paths.objs);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this meant to be console.error?

Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet left a comment

Choose a reason for hiding this comment

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

:shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dash 1.11 suppresses unrelated callbacks on component-load if ReferenceError detected
2 participants