Skip to content
This repository has been archived by the owner on Nov 1, 2019. It is now read-only.

Task Modifiers #6

Open
buschtoens opened this issue Jun 15, 2019 · 4 comments
Open

Task Modifiers #6

buschtoens opened this issue Jun 15, 2019 · 4 comments

Comments

@buschtoens
Copy link
Owner

Unfortunately we cannot support task modifiers as easily as we planned initially in machty/ember-concurrency-decorators#50.

class Foo {
  doStuff = task(function*(this: Foo) {
    // ...
  }).restartable();

  perform() {
    this.doStuff.perform();
  }
}

Our type definition currently basically looks like this:

declare module 'ember-concurrency' {
  export function task<Args extends any[], R>(
    task: GeneratorFn<Args, R>
  ): Task<Args, Exclude<R, Promise<any>>>;

  export interface TaskProperty {
    restartable: () => TaskProperty;
    drop: () => TaskProperty;
    keepLatest: () => TaskProperty;
    enqueue: () => TaskProperty;
    maxConcurrency: (n: number) => TaskProperty;
    cancelOn: (eventName: string) => TaskProperty;
    group: (groupName: string) => TaskProperty;
    evented: () => TaskProperty;
    debug: () => TaskProperty;
    on: (eventName: string) => TaskProperty;
  }

  export interface Task<Args extends any[], T> {
    readonly isIdle: boolean;
    readonly isQueued: boolean;
    readonly isRunning: boolean;
    readonly last?: TaskInstance<T>;
    readonly lastCanceled?: TaskInstance<T>;
    readonly lastComplete?: TaskInstance<T>;
    readonly lastErrored?: TaskInstance<T>;
    readonly lastIncomplete?: TaskInstance<T>;
    readonly lastPerformed?: TaskInstance<T>;
    readonly lastRunning?: TaskInstance<T>;
    readonly lastSuccessful?: TaskInstance<T>;
    readonly performCount: number;
    readonly state: TaskState;
    perform(...args: Args): TaskInstance<T>;
    cancelAll(): void;
  }

  export interface TaskInstance<T> extends PromiseLike<T> {
    readonly error?: unknown;
    readonly hasStarted: boolean;
    readonly isCanceled: boolean;
    readonly isDropped: boolean;
    readonly isError: boolean;
    readonly isFinished: boolean;
    readonly isRunning: boolean;
    readonly isSuccessful: boolean;
    readonly state: TaskInstanceState;
    readonly value?: T;
    cancel(): void;
    catch(): RSVP.Promise<unknown>;
    finally(): RSVP.Promise<unknown>;
    then<TResult1 = T, TResult2 = never>(
      onfulfilled?:
        | ((value: T) => TResult1 | RSVP.Promise<TResult1>)
        | undefined
        | null,
      onrejected?:
        | ((reason: any) => TResult2 | PromiseLike<TResult2>)
        | undefined
        | null
    ): RSVP.Promise<TResult1 | TResult2>;
  }
}

The issue is that task returns a Task, so that can call perform etc. on the class property.

However, this means that you can't call .restartable() etc. (from TaskProperty) in the class property assignment.

I see the following solutions:

  • Change task to return a Task & TaskInstance, which loosens the type safety. 😦
  • Allow usage of modifiers as decorators, e.g.:
    @restartable
    someTask = task(function*() { ... });
  • Accept modifiers as an options object, ember-concurrency-decorators style:
    someTask = task({ restartable: true }, function*() { ... });
  • Allow chainable API, but require a "terminator":
    someTask = task(function*() { ... }).restartable().task;
    regularTask = task(function*() { ... }).task;
  • "Inline" chainable API:
    someTask = task(function*() { ... }, t => t.restartable());
    or
    someTask = task(function*() { ... }, task.restartable());
@makepanic
Copy link

Thanks for creating this addon.

We're currently using ember-concurrency-decorators so I'd prefer the concurrency-decorators style.

It has the advantage, that the task definition is in one place, instead of being split by it's implementation.

@gossi
Copy link

gossi commented Jun 28, 2019

I don't mind doing this:

@task({...})
someTask = task(function*() {...});

@restartableTask({...})
someOtherTask = task(function*() { ... });

I do like it for the reason, that at some point we will have this:

@task({...})
someTask*() {...}

The "the upper part" (= decorators) will mostly stay the same for what we write today and in the future, so no relearning here. While we (as TS users) know, that we cannot use method*() for now but have to use "workaround syntax" which is method = task(function* () {...});.

I dunno how often syntax changed over the past 2-3 month. At least this sounds like an approach that will consistency.

Although, yes - you may have a duplicate task here to use the same decorator as task function to give options.

PS. I'm not clear about the current state of e-c and whether task can be used as fn and decorator but I assume it to be possible 🙈

@dfreeman
Copy link

I'm generally a fan of accept modifiers as an options object, ember-concurrency-decorators style, but it's not clear to me how that would interact with libraries that add additional methods on TaskProperty.prototype, like ember-concurrency-retryable.

I haven't looked at the implementation as it stands today, but do any of these approaches (putting aside decorators) allow for using augmentations that are made to the base ember-concurrency implementation without needing special knowledge of ember-concurrency-typescript?

@buschtoens
Copy link
Owner Author

buschtoens commented Jun 28, 2019

@dfreeman ember-concurrency-decorators itself has no implicit knowledge of ember-concurrency internals. applyOptions just iterates over all entries of the options object and if the value is true plainly calls the method with that key or for any other case passes the value as the first argument. This way, extensions to the TaskProperty.prototype work out of the box.

The implicit limitation however is, that you can at most pass one argument and the argument itself cannot be true.

This means a hypothetical extension like throttle(spacing: number, immediate: boolean) would currently not be supported. We could make it work by spreading arrays, e.g.

myTask = task({ throttle: [100, true] }, function*() {});

Writing this I just realized, that we actually need something like this, to support .on(eventNames...) and .cancelOn(eventNames...), e.g.

myTask = task({ on: ['init', 'click'] }, function*() {});

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants