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

1519 callbacks waiting fix #2344

Merged
merged 6 commits into from
Dec 8, 2022
Merged

1519 callbacks waiting fix #2344

merged 6 commits into from
Dec 8, 2022

Conversation

eff-kay
Copy link
Contributor

@eff-kay eff-kay commented Nov 30, 2022

Fixes #1519

the description of what this PR does is found in this comment

#1627 (comment)

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
    • task 1
    • task 2
  • 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 follows
    • this GitHub #PR number updates the dash docs
    • here is the show and tell thread in Plotly Dash community

@eff-kay
Copy link
Contributor Author

eff-kay commented Nov 30, 2022

@alexcjohnson rebasing #1627, took way longer than expected, so I simply created a new branch. This is way cleaner and easier to test. Marc-Andres comment is pretty much what I am doing to fix the issue, in that I traverse the entire tree and filter out duplicate calls.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 Thanks @eff-kay, looks good to me! I refactored your test a little, to sequence based on a Lock rather than delays and to look at number of callback invocations to ensure we aren't missing an intermediate state - but I'm pretty sure it still tests the same behavior.

@eff-kay
Copy link
Contributor Author

eff-kay commented Dec 7, 2022

💃 Thanks @eff-kay, looks good to me! I refactored your test a little, to sequence based on a Lock rather than delays and to look at number of callback invocations to ensure we aren't missing an intermediate state - but I'm pretty sure it still tests the same behaviour.

@alexcjohnson I verified the test, it validates the correct thing. I have one small comment on understanding how locks work, other than that feel free to merge the PR.

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.

[BUG] Dependent callbacks are not waited and executed in order
2 participants