Allow task to work as a "pass-through" wrapper for TS support

[jamescdavis]

This is based on #56 now that types have landed in ember-concurrency.

The idea is that task can act as both a decorator and as a function that wraps a task generator or encapsulated task descriptor. When used as a wrapper, it simply passes through the argument while providing a return type of Task. This allows you to interact with the task property exactly as you would in JavaScript, but with the type-safety of TypeScript:

import { task } from 'ember-concurrency-decorators';

class Foo {
  @task
  myTask = task(function*(arg: string) {
    yield;
    return 100;
  });

  doSomething() {
    this.myTask.perform(); // $ExpectError
    this.myTask.perform(99); // $ExpectError
    this.myTask.perform('Tada!'); // 🎉 
  }

  getVal() {
    const lastVal1: string | null | undefined = this.myTask.last?.value; // $ExpectError
    const lastVal2: number | null | undefined = this.myTask.last?.value; // 🎉 
  }
}

Note: This is an alternative approach to https://github.com/chancancode/ember-concurrency-ts

@chancancode @dfreeman @chriskrycho @buschtoens

[chriskrycho]

FWIW, I very much prefer this approach! It avoids introducing any other new API surface, and looks like it should be easy to iterate toward a “resources”-based approach (the @use proposal). cc. @chancancode @maxfierke @dfreeman @pzuraq

[dfreeman]

I'm also a big fan of this, particularly if ember-concurrency-async can be updated to work with it!

Those two things together would deliver end-to-end type safety without putting the burden on the caller to remember to use taskFor anywhere they want to interact with a task ✨

[jamescdavis]

When machty/ember-concurrency#363 lands, we can remove this ugly special-casing for the 2.18 try case and just use, e.g.

assert.equal(subject.get('doStuff').get('last')?.value, 123);

[jamescdavis]

Couldn't resist: b5dbcb4

[jamescdavis]

chancancode/ember-concurrency-async#6 makes this work with async tasks e.g.

class MyClass {
  one = 1;

  @task
  myTask = task(async function(this: MyClass) {
    await this.one;
    return 2;
  });

  @task
  myTask = task(async () => {
    await this.one;
    return 2;
  });
}

[jamescdavis]

AsyncTask types and tests for async tasks w/ task wrapper are here: jamescdavis#1 but we obviously can't drop them in like that. Need to think about how to make them manually importable like ember-concerrency-asyncs are.

[maxfierke]

Overloading task in this way and using class field syntax is a bit confusing to me, as I'm not sure there's really a need for the decorator in that context?

If you just use class field syntax without using the decorator and using task provided by ember-concurrency 1.2.0 itself, does it not properly type?

[chriskrycho]

It would type correctly but behave in an undesirable ay: assignment to a class field is a per-instance setup, unlike methods which end up on the prototype. Decorators can transform properties or methods, and can in fact transform the property into a prototype-bound method. Using the two in combination is definitely odd, but it’s what’s necessary for now.

It’s also close to what we’ll likely end up with in a resources-driven design in the future, where a task usage might look something like this:

import Component from '@glimmer/component';
import { use } from '@ember/something-something-resources';
import { task } from 'ember-concurrency';

export default class MyComponent extends Component {
  @use myTask = task(function*() {
    // ... the normal task body ...
  });
}

[jamescdavis]

What @chriskrycho said and it throws an error: https://ember-twiddle.com/55d9db44e9958d7fab70fe1fa91e218b?openFiles=components.undecorated-task%5C.js%2C

[jamescdavis]

The double usage of task is a little different, I agree, but the alternative would be to introduce a new thing to import, which seems not great and is further from the @use pattern @chriskrycho mentioned. Also, not sure what we would call it.

[jamescdavis]

Some more thoughts about this design that I had a when I proposed it @buschtoens:

• it is syntactically very close to the pre-ES6-classes incantation:

  export default Component.extend({
    myTask: task(function*() {});
  });

  ||
  V

  export default class MyComponent extends Component {
    @task
    myTask = task(function*() {});
  }

  making it easier for people migrating to ES6 classes/Octane.
• A number of the decorators now built-in to Ember core serve "double duty" and can be used as a decorator and a regular function, e.g.

  import { computed } from '@ember/object';
  
  export default class extends Component {
    @computed('something')
    get doubleSomething() {
      return this.something * 2;
    }
  }

  and

  import { computed } from '@ember/object';
  
  export default Component.extend({
    doubleSomething: computed('something', function() {
      return this.something * 2;
    }
  });

  This is obviously not the same thing because, in the latter, computed actually does something rather than pass through and in what I'm proposing here, we'd use task as a decorator and a function at the same time, but I just wanted to point out that the concept of an import switching behavior based on usage is not new in Ember.

[maxfierke]

I don't think it's the overloading of task that I have concerns about so much as it's task from ember-concurrency-decorators being overloaded and not task from ember-concurrency. It's becoming clearer from the past few months of more people adopting ES classes and using decorators with ember-concurrency, that it's probably inevitable that we merge the decorator approach here into upstream ember-concurrency at some point. So my concern is that this approach might lock us out from doing that or make that usage more confusing, if it is possible to retain these semantics in some clever merging. Is the function wrapping here at all in conflict with task from ember-concurrency?

I do find the @use use-case (lol) compelling for this though!

[jamescdavis]

So, you're thinking of making import { task } from 'ember-concurrency'; also work as a decorator? Yeah, that would conflict because we'd have no way to tell whether a functional invocation of task should just pass through the generator function or create a task with it. In that case, we'd have to call this pass-through-wrapper-for-types thing something else. Any ideas for a name?
If only decorators could affect type...

[jamescdavis]

getTask()?
makeTask()?
typeAsTask()?
makeThisThingATaskAsFarAsTypeScriptIsConcerned()?

[jamescdavis]

trustMeItsATask()?

[maxfierke]

I think my preferences would lie with something like castToTask, toTask, or asTask? Obviously, these are all a bit longer to type than task, but perhaps someone will figure out a nice import alias.

[jamescdavis]

Now that you mention it, I believe asTask was my original suggestion before we landed on just overloading task. That fits well with TypeScript's as keyword for asserting type, which is pretty close to what we're doing here. It's basically the same as:

@task
myTask = (function*(something: string) {
  yield something;
  return 42;
} as unknown) as ComputedProperty<Task<number, [string]> & EmberObject>;

except that it automatically infers the return vale and arguments and isn't ugly as hell.

What do we think about?:

@task
myTask = asTask(function*(something: string) {
  yield something;
  return 42;
});

React with: 👍 , 👎, or 😕

[chancancode]

As far as I can tell, if we are not overloading, then that is pretty much the same signature as taskFor, and if you prefer to put taskFor around the assignment instead of on the usage side, it will Just Work™? Not opposed to exploring this or merging the addons (it certainly doesn't make it easy for me to maintain), but checking if I am missing something.

[jamescdavis]

@chancancode well, actually, yeah! taskFor does exactly the same thing if we're not overloading and could definitely be used at assignment (which is very much my preference and I believe that of some others but I don't want to speak for them). It would be nice if it were part of ember-concurrency-decorators so there wasn't another package to install and IMHO the name is not as clear as to what it does as asTask, but otherwise this works fine:

import { taskFor } from 'ember-concurrency-ts';

export default class MyComponent extends Component {
  @task
  myTask = taskFor(function*(something: string) {
    yield something;
    return 42;
  });
}

as well as

import { taskFor } from 'ember-concurrency-ts';

export default class MyComponent extends Component {
  theAnswer = 42;

  @task
  myTask = taskFor(async (something: string) => {
    await something;
    return this.theAnswer;
  });
}

when coupled with a slightly adjusted chancancode/ember-concurrency-async#6.

So maybe there's actually nothing new here. ¯\_(ツ)_/¯

I would like to see this usage illustrated as an option on http://ember-concurrency.com/docs/typescript (and in ember-concurrency-ts).

[jamescdavis]

Well, works fine for types. We'd have to adjust this assertion for taskFor to work for assignment: https://github.com/chancancode/ember-concurrency-ts/blob/05795edb1ae8cd5f6818d36aaf2ca9cb74d4f5f4/addon/index.js#L6

Here's a PR that does that: chancancode/ember-concurrency-ts#1

[jamescdavis]

Here's a PR that makes using taskFor at assignment work with async: chancancode/ember-concurrency-async#7 (tests won't pass without chancancode/ember-concurrency-ts#1 but the transform works).

[jamescdavis]

Closing in favor of chancancode/ember-concurrency-ts#1