-
-
Notifications
You must be signed in to change notification settings - Fork 42
Clientside callbacks #30
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
Conversation
@test app.callbacks[Symbol("my-div.children")].func("value") == "value" | ||
@test app.callbacks[Symbol("my-div2.children")].func("value") == "v_value" | ||
|
||
@test_throws ErrorException callback!(app, callid"my-id.value => my-div2.children") do value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me exactly what's being tested by some of these @test_throws
- at a minimum it would be nice to have a comment like "my-div2.children is already an output", but better would be for the code to somehow inspect the error to ensure it's actually the one you intended to be thrown, as opposed to something else like a typo in the callid
string causing a regexp failure.
Minor, and not new in this PR, you're just rearranging, but it's the first I'm looking at this code 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, very clean. No comments on the source code, and aside from my one minor comment on the exception testing, all I noticed is it's perhaps overkill to copy all the Python clientside callback tests here.
I wouldn't bother pruning them, but as a general rule all we need to test here is the behavior of the back end. Once a few clientside callbacks (single/multi output, with or without state...) have made it to the front end, everything else is testing specific behavior of the renderer, and we can assume that's tested in the main dash repo, where the renderer code lives. Aside from the extra time to port the tests to Julia it's both extra test execution time and more maintenance cost should any of those tests need to change in the future.
Anyway neither of these comments is blocking - nice job! 💃
@alexcjohnson About copying Python tests. I thought about this, but decided to continue copying its for the following reason. I have very little experience of using dash on the user's side, so I assume that Python tests handle some of the pitfalls that Python users have already encountered. Plus for me, this is a kind of tutorial on how dash is used. With exceptions, the problem is more global - I created a separate issue(#37) for it |
ClientSide callbacks.
Allows you to add Client Side callbacks using
or
Based on
hot_reload
branch and should be reviewed after itCorresponding integration tests: https://github.com/plotly/Dash.jl/blob/clientside_callbacks/test/integration/clientside/test_clientside.py