machty / ember-concurrency

ember-concurrency is an Ember Addon that enables you to write concise, worry-free, cancelable, restartable, asynchronous tasks.
http://ember-concurrency.com
MIT License
689 stars 157 forks source link

Common Patterns Discussion #72

Open aaronbhansen opened 8 years ago

aaronbhansen commented 8 years ago

We have a few common patterns we notice happening and wanted to bring them up in a discussion to see if its anything you think would be helpful adding in as a default operation.

1. Unwrapping promises passed in

Most of our tasks start with something like:

someTask: task(function * (one, two, three) {
  one = yield one;
  two = yield two;
  three = yield three;

Since we are working with promises or previously fulfilled promises, we want to unwrap them so we have access to the methods not just through the proxy object on an ember data promise. It would be great to have all the parameters already unwrapped. Any thoughts or ideas on this?

2. Instance based isRunning task property

Since tasks live on an object and once that object dies, all the tasks die. It would be great to have something similar a global property that checks if any task on that object is running. We usually break up code into logical tasks methods, but we create a property like this on the object to see if any task is running.

isRunning: Ember.computed('taskOne.isRunning', taskTwo.isRunning', taskThree.isRunning', ...

That way if any task is running on that object the UI can reflect a processing state. Maybe there is a better alternative you've already found.

Thanks for ember concurrency, it has totally changed how we structure our ember apps.

machty commented 8 years ago

Thanks for the feedback, I'm definitely trying to collect all the use cases as to how people are using e-c.

1. Unwrapping promises passed in

I haven't personally run into this pattern too often, and I don't think I've heard anyone else bring it up. Perhaps you could supply a gist that provides a little more context as to who's calling .perform() with unresolved promises so I can get a better sense of the pattern?

The idea of building this into e-c reminds me of another API I've flirted with but ultimately wasn't comfortable adding: the pattern of doing a timeout at the beginning of a .restartable() task as a means to debounce a task is so common that you'd think there'd be a .debounce(ms) modifier. The only reason I didn't add it is that I felt it'd complicate the story with .isRunning / .isIdle and other kinds of derived state, e.g. is a task considered to be running if it hasn't surpassed its debounce time? Should it expose some other state when the "real" underlying task is actually running? Similar questions would arise if we built in auto-promise-arg-resolution. Another issue is when it comes to task modifiers like restartable(), should we restart the task on every perform(), or only when the promise args passed to perform have been resolved?

It's possible we can come up with solutions to all of these but I haven't settled on a concept simple enough to understand/explain.

2. Instance based isRunning task property

Hmm, this might be a good idea. Perhaps something like:

import { task, areTasksRunning } from 'ember-concurrency';

export default Ember.Component.extend({
   task0: task(...),
   task1: task(...),
   task2: task(...),
   task3: task(...),
   task4: task(...),

   // use magic computed property provided by e-c that evals
   // to true / false if any task on the object is running
   showSpinner: areTasksRunning(),
});

One drawback to this idea is that there are still some questions in the air regarding task "ownership", i.e. there are semantic differences between a component .perform()ing a task that was passed into it vs a task declared on the component class. The component "owns" the task in the latter case and hence the task is automatically canceled upon object destruction/component unrendering, but the passed in task would not be canceled (unless it was performed from a task owned by the component, in which case it'd be canceled in a chain reaction manner). If we were to implement something like tasksAreRunning(), the easiest way to implement it would be to assume that it was only "owned" tasks living on the component and not a task that might have been passed in, but this might be confusing in practice.

Generally speaking though I think this is a yet-unsolved problem with e-c in general and nothing specific to this proposal. I'd definitely like to hear some feedback from other people that might want a feature like this.

aaronbhansen commented 8 years ago

Sounds good on 1, we end up passing a lot of possible promises into tasks, and its easier to ensure they are all unwrapped at the start so we don't have to check later, but we can keep manually doing that for now.

On number 2, I agree there is some issues with ownership. If others could give their feedback, it would help clarify usage patterns. The more our code is broken into logical separation for tasks, the more we seem to need this pattern to see if any of the tasks on the current object is running.

sukima commented 8 years ago

the pattern of doing a timeout at the beginning of a .restartable() task as a means to debounce a task is so common that you'd think there'd be a .debounce(ms) modifier

I would argue against this. Mainly because the boilerplate that this sugar would try to hide is so small that not having the sugar would increase readability and comprehension.

While my co-worker and I was learning e-c being forced to yield a timeout forced us to comprehend the state of things. And to understand what restartable did. This helped us immensely in growking coroutines.

If the pattern were more complex then I would say some sugar might help but this instance is so minor that doing it by hand really helps you with knowledge retention. Like a manual stick shift vs automatic cars, you would not realize how gear ratios works without a manual stick and yet the two interfaces are almost the same.

sukima commented 8 years ago

Unwrapping promises passed in

Never ran into this. In fact if I were to run into this I would immediately think to unwrap the promise in the caller not the task.

Perhaps this is a case for exposing a low-level coroutine function that would allow generator functions to be executed in contexts outside of a task. (disclosure: this is something I'm working on in a future issue proposal). See Issue #80

sukima commented 8 years ago

@aaronbhansen what about this:

import { task, all } from 'ember-concurrency';

task(function * (...promises) {
  let [one, two, three] = yield all(promises);
  …
})

I don't quite see the need to auto unwrap. feels like this is out of scope for e-c and more in scope with coding style.

aaronbhansen commented 8 years ago

Your comment on manual vs automatic made me think that there could be an alternative task method to unwrap any parameters passed in. Those who want to do it a the current way can and those who want everything yielded up front can call this other method.

import { task } from 'ember-concurrency'; and import { taskAuto } from 'ember-concurrency'; (Bad name, just as an example)

Mostly we yield everything at the start of the method since its so much easier than outside of a task