Skip to content

perform_cmd and Cancellation #130

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

Closed
theduke opened this issue Jun 7, 2019 · 3 comments · Fixed by #380
Closed

perform_cmd and Cancellation #130

theduke opened this issue Jun 7, 2019 · 3 comments · Fixed by #380
Labels
API design Proposal for a new or updated Seed API.

Comments

@theduke
Copy link

theduke commented Jun 7, 2019

Currently, if you want cancellation with perform_cmd, it has to be implemented manually in a rather awkward way.

It would be great if perform_cmd returned a drop guard which, when dropped, drops the corresponding future and prevents a message to be sent.

@theduke theduke changed the title perform_cmd and cancellation perform_cmd and Cancellation Jun 7, 2019
@MartinKavik
Copy link
Member

I like this, so some ideas:

Let's use new Api from #131 :

orders
    .subscribe(Sub::interval(1000, Msg::Tick));
    .perform_cmd(Cmd::timeout(500, Msg::Delayed));

We can implement their non-chainable versions:

let sub_handle = orders.subscribe_with_handle(Sub::interval(1000, Msg::Tick));
let cmd_handle = orders.perform_cmd_with_handle(Cmd::timeout(500, Msg::Delayed));

And then:

// use it before the subscription is registered
sub_handle.cancel();
// use it if you don't want to receive more events
sub_handle.unsubscribe();

// use it before the command is executed
cmd_handle.cancel();
// use it if you want to abort already executed command
cmd.handle.abort();

Maybe unsubscribe and abort will be enough.
I personally don't like dropping in this case because it seems to me a little bit implicit and error prone.


Is it possible to get "raw" handle for specifix subs/cmds? E.g. IntervalId from Interval, TimeoutId from Timeout, RequestController from FetchRequest, etc.?

If we use this concept + subscriptions for window_events it should(?) resolve problem with reregistration window listeners:

// vdom.rs:686 and vdom.rs:449
/// We reattach all listeners, as with normal Els, since we have no
/// way of diffing them.
fn setup_window_listeners<Ms>(...

@theduke
Copy link
Author

theduke commented Jun 7, 2019

I personally don't like dropping in this case because it seems to me a little bit implicit and error prone.

I have a different view here. I'd much prefer a dropguard-by-default API. (with a explicit opt-out if you need it, eg: .perform_cmd().keep_alive().

It's slightly inconvenient, but with drop guards, you are forced to encode your effect based UI logic into the type system, since if you don't store a handle it is immediately dropped and never triggers.

This imo leads to much more descriptive and maintainable code.

A major use case for me would be automatically dropping subscriptions and cancelling futures when parts of the model are dropped. In that case you would just have to again manually implement drop for your type.

A classic example:

struct ListView {
  current_request: Option<Handle>,
  filter_debounce: Option<TimeoutHandle>,
}

enum View {
  Home,
  List(ListView),
}

struct Model {
  view: View,
}

fn update(..) {
  match msg {
    Msg::ShowHome => { 
      // ListView is dropped here and everything is cleaned up.
      self.view = Home;
    }
  }
}

@MartinKavik
Copy link
Member

Yeah, good point. I like the idea with keep_alive() - it's a good compromise.

My simple example why not to drop it:

  1. A user fill out a long and important form on the ListView.
  2. Then click on the Save button.
  3. Then click on "back to Home".
  4. => Request is aborted and user is angry.

But you are probably right and there are only a few cases like this - keep_alive should be enough,

@MartinKavik MartinKavik mentioned this issue Mar 7, 2020
@MartinKavik MartinKavik added this to the 2. Stable basic Seed API milestone Mar 9, 2020
@MartinKavik MartinKavik added the API design Proposal for a new or updated Seed API. label Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API design Proposal for a new or updated Seed API.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants