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

Remove Event system #531

Closed
alexcjohnson opened this issue Jan 9, 2019 · 5 comments
Closed

Remove Event system #531

alexcjohnson opened this issue Jan 9, 2019 · 5 comments

Comments

@alexcjohnson
Copy link
Collaborator

Expanding on #469 (comment):

Perhaps we can remove events here (in dash 1.0) as well?

We've had some offline discussions about this, but this deserves a more public issue both to allow any further input folks might have, and to tie to forthcoming PRs on the topic.

Two parallel callback trigger systems: Input and Event

Most interactions in dash are handled by properties, referenced by either Input or State as callback parameters, or by Output as the result of a callback. But there's also an Event system, which can trigger a callback without a value, just based on some event occurring. This system was originally made to handle user interactions with no inherent visible consequences (so they don't have an obvious mapping to stateful properties), like button clicks. But for some time now we've had another way to handle this kind of scenario in a stateful way, which has all the same capabilities and more without needing a parallel system, and a lot of our newer components have no events defined at all. Dash 1.0 is the right time to remove Event entirely and only accept the property-based alternative.

Transition from Event to Input

The key is to create a property that the component modifies every time the event occurs, so that the appropriate callback will fire on every event. We've generally done this by incrementing a counter. The most widely used right now in our codebase is the click event, where instead of emitting a 'click' event we create a 'n_clicks' property. One complication of this is that in the property version, the callback will be fired on initial render with n_clicks=0. To get around this, you can use PreventUpdate:

@app.callback(Output('output', 'children'), [Input('button', 'n_clicks')])
def set_output(n_clicks):
    if not n_clicks:
        raise PreventUpdate
    return 'You clicked the button!'

An issue that we never had a simple solution for with Event was multiple events triggering the same callback, for example two buttons that should cause the same output component to behave differently. We have another set of properties you can use to handle this, based on timestamps. By convention these properties get the same name as the counter, suffixed with _timestamp - n_clicks_timestamp for example, and this would contain the timestamp (milliseconds since 1970) when the latest click occurred:

@app.callback(Output('output', 'children'),
              [Input('button1', 'n_clicks_timestamp'), Input('button2', 'n_clicks_timestamp')])
def set_output(ts1, ts2):
    if ts1 < 0 and ts2 < 0:
        # before any click, timestamp is -1 by convention
        raise PreventUpdate
    if ts1 > ts2:
        return 'You clicked button 1 most recently'
    else:
        return 'You clicked button 2 most recently'

I'll make corresponding issues in the other core repos to track any specific aspects of this relevant there and to link to PRs.

@alexcjohnson
Copy link
Collaborator Author

Corresponding issues in the other core repos:
plotly/dash-renderer#113
plotly/dash-core-components#433
plotly/dash-html-components#88

@mungojam
Copy link
Contributor

We prefer using Event for things like button clicks as it needs less explanation for people who aren't so familiar with Dash. The n_clicks and timestamp methods also feel like tricks or workarounds much like hidden div did until you created store.

Until this point I had assumed that Event was as a new thing that was being added to replace the other tricks.

I may be alone in this though in which case, we'll stop using Events

@alexcjohnson
Copy link
Collaborator Author

@mungojam from our side, Event is actually an older feature, introduced before we realized the same thing could be accomplished with a property. I can understand the perspective that a click event is more intuitive than an n_clicks property, though in one sense that's a feature: it's a fairly straightforward introduction to the mode of thinking of everything about your app as stateful properties, and the transition between the two is easy.

That said it may be worthwhile discussing as part of this issue whether or not we really want the callback to fire on initialization - if we skipped this, switching from an event to the corresponding property really would be as easy as changing the argument. @plotly/dash any thoughts about this? Is there a reason to keep the zero-state callback for event-derived properties?

Note that timestamps aren't necessarily part of this discussion at all - the possibilities they open were not available at all with events, so we would have needed to add them, or something else to accomplish those goals, regardless of our approach with events. That said, if we do use timestamps as a property it seems even more incongruous to have the trigger itself be a non-property event.

From my perspective the benefit of removing events is primarily simplicity:

  • There's one right way as a user to accomplish your goal, which means less to learn overall and it's easier for users to help each other.
  • There's only one system for us to maintain, and extend as we add other new features like multi-output and dynamic callbacks. There are currently some skipped tests, at least in dash-renderer, indicating that events are partially broken right now, but these tests work just fine when converted to properties.

@chriddyp
Copy link
Member

I agree with everything that @alexcjohnson mentions above.

In addition, removing events is easier for component authors as there is less ambiguity about "what should be an event". For example, clicking on points in a graph: with stateful properties, it's clickData but if you had to think about events, you might feel compelled to add a "click" event. However, from the end user's perspective, a "click" event on a graph isn't useful without the click data.

With respect to documentation and "how new is event?", Event is something that we stopped documenting around the summer. However, folks are still using it from community forum examples I imagine.

For callback firing on initialization, I think that gets to the default properties discussion here: #468. For event-driven properties, we don't always want to skip the initial callback. Consider a figure depending clickData - it still needs to initialize before its clicked on.
Another option would be to allow the users to skip the initialization parameter within the decorator, like:

@app.callback(...)
@app.skip_initial_update
def update_graph(...)
   # ...

for now, the official way is to raise PreventDefault:

@app.callback(...)
def update_content(n_clicks)
   if n_clicks is None:
       raise dash.exceptions.PreventDefault

which I agree is tedious, however it is consistent with how the rest of the system works.

@mungojam
Copy link
Contributor

That all makes sense. I guess I prefer the dialect I made up with the addition of Source() so you wouldn't need the timestamp to identify the source component. But I totally get that you are trying to minimise the number of concepts.

A bit OT but I'm intrigued to see how client side callbacks turns out because the proposal seems to be closer to the style I'd like for python callbacks too

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

No branches or pull requests

3 participants