Add type definition based on API docs

[chancancode]

Yet another attempt at adding types.

Quite similar to #355, but this includes every item (and only those items) in the public API documentation. Also uses interfaces over classes, since the e-c classes are not meant to be instantiated manually or subclassed by apps.

[NullVoxPopuli]

Lgtm. Idk if I'll have time to test. But having something to iterate on is better than not having anything / each of us copy pastin d.ts everywhere

[machty]

This lgtm to I'm behind on the TypeScript discussion / tradeoffs when it comes to e-c so I'll defer to others.

[maxfierke]

LGTM, with the caveat about the commented out TaskGroup stuff indeed being public. But that's the only blocker for me.

[chancancode]

@maxfierke I have added (mostly just copied) the docs for TaskGroup

[maxfierke]

@chancancode I think this looks great! Before I approve, are there any caveats you can think of that would be worth mentioning or potentially breaking for folks (outside of people using their own types)? I'm thinking specifically about TypeScript not supporting changes to the return types of generators at one point, IIRC.

[chancancode]

Before I approve, are there any caveats you can think of that would be worth mentioning or potentially breaking for folks (outside of people using their own types)?

These PR adds the following top level type symbols:

• TaskGenerator
• TaskFunction
• TaskFunctionArgs
• TaskFunctionReturnType
• Task
• TaskGroup
• TaskInstance
• TaskProperty
• TaskCancelation
• Yieldable
• Evented

These can be imported from the ember-concurrency module by name when using TypeScript, so they are effectively public API once released.

Because they are type symbols, non-TypeScript projects won't be able to import them, and TypeScript projects won't be able to do anything with them other than using them as types (e.g. you cannot do new Task() because that would require Task being a "value" symbol, but Task is just a type here (which is erased from the code once compiled).

These names are effectively "reserved". It would be okay if, for example, ember-concurrency wants to export the same, currently internal, Task class as a public API for subclassing. In that case, we will just update the TypeScript definition from an interface to a class, all the existing properties will match up just fine. However, if ember-concurrency wants to use the Task named export for something completely different (say, export cons Task = "Task, the string.";), that would not be okay since the type definition will be forced to find a different name for what is currently exported as Task, and that would be a breaking change.

In practice, I think this seems unlikely to be a problem since I was quite careful to pick the names to match the defacto public names in the documentation (@class Task, etc). It's also possible to reclaim these symbols for something else by bumping major version.

I'm thinking specifically about TypeScript not supporting changes to the return types of generators at one point, IIRC.

I think this is referring to the fact that the decorator cannot change the type of the property it's decorating. So since most people write @task *foo()... you cannot convince typescript to let you call this.foo.perform(...) no matter what.

This is not really an ember-concurrency problem since the decorators are outside of this repository. Based on the way I typed it, the classic style foo: task(function *() { ... }) should work _when used with this.get('foo').perform(...). I think this.foo.perform(...) still won't work there since the task function is typed to return a TaskProperty.

The workaround in our project (very similar to @NullVoxPopuli's) is the following utility functions:

// app/types/ember-concurrency.d.ts

import {
  Task,
  TaskFunction,
  TaskFunctionArgs,
  TaskFunctionReturnType,
  TaskInstance
} from 'ember-concurrency';

/**
 * No-op typecast function that turns what TypeScript believes to be a
 * generator function into a Task.
 *
 * ```js
 * import { taskFor } from 'direwolf/types/ember-concurrency';
 *
 * class Foo extends EmberObject {
 *   @task *myTask() {
 *     // ...
 *   }
 *
 *   someMethod() {
 *     this.myTask.perform(); // TypeError
 *     taskFor(this.myTask).perform(); // ok!
 *   }
 * }
 * ```
 *
 * @param task The task. Note that this is purely a typecast function,
 *   it does not in affect accept a task generator function as input.
 */
export function taskFor<T extends TaskFunction<any, any[]>>(task: T):
  Task<TaskFunctionReturnType<T>, TaskFunctionArgs<T>>;

// app/types/ember-concurrency.js

import { assert } from '@ember/debug';

export function taskFor(task) {
  assert(
    `${task} does not appear to be a task!`,
    task && typeof task.perform === 'function'
  );

  return task;
}

In actual usage:

// @ts-check
// app/components/foo.js

import { action } from '@ember/object';
import Component from '@glimmer/component';

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

import { taskFor } from 'my-app/types/ember-concurrency';

export default class extends Component {
  /**
   * @param {string} foo
   */
  @task *myTask(foo) {
    // ...
    return 1;
  }

  async someMethod() {
    this.myTask.perform("foo"); // error
    this.myTask.isRunning; // error

    taskFor(this.myTask).perform("foo"); // ok
    taskFor(this.myTask).perform(); // error
    taskFor(this.myTask).perform(123); // error

    let foo: number = await taskFor(this.myTask).perform("foo"); // ok
    let bar: string = await taskFor(this.myTask).perform("foo"); // error

    taskFor(this.myTask).isRunning; // ok

    let task = taskFor(this.myTask);

    task.perform("foo"); // ok
    task.perform(); // error
    task.perform(123); // error

    foo = await task.perform("foo"); // ok
    bar = await task.perform("foo"); // error

    task.isRunning; // ok
  }
}

[maxfierke]

@chancancode thanks! that all makes a lot of sense.

Two questions:

• would you be willing to add a page to the docs w/ that example?
• would it make sense to include such a type-cast function as part of this?

[chancancode]

would you be willing to add a page to the docs w/ that example?

Sure, but I'll need a couple of pointers:

• Is there an inline way to write code snippets, or do I just make the file in the snippets folder?
• Does the "snippets" support .ts or .d.ts?
• Should I be using decorators in the example or using classic syntax? I suppose the latter could work, but it doesn't work super well with TypeScript, but perhaps it works good enough for the purpose of the example.
• Glimmer or classic component?
• For classic syntax at least, I believe .get is supposed to Just Works™ here, should I be using that?

would it make sense to include such a type-cast function as part of this?

I'm not sure. For one, it's kind of weird to have an empty function that doesn't do anything for non-TypeScript users. The other is that it's basically an "unsafe" function that works by lying to the compiler to mask the type error. I don't think it's particular dangerous, because at the end of the day it's just a weird quirk/fallout, but I'm not sure it's good to include it as an official API.

[maxfierke]

• Is there an inline way to write code snippets, or do I just make the file in the snippets folder?

Yep, you can do this in HBS:

{{! BEGIN-SNIPPET some-template-name }}

{{! END-SNIPPET }}

or in JS/TS

// BEGIN-SNIPPET

// END-SNIPPET

It uses https://github.com/ef4/ember-code-snippet underneath, so anything mentioned in the docs should apply within e-c (though, we use 2.4.x, so we do have syntax highlighting and do not have the get-code-snippet helper mentioned on the main page)

• Does the "snippets" support .ts or .d.ts?

Yep, it should.

• Should I be using decorators in the example or using classic syntax? I suppose the latter could work, but it doesn't work super well with TypeScript, but perhaps it works good enough for the purpose of the example.

Anecdotally, it seems most folks using TypeScript are using it with decorators, so we can probably stick to those.

• Glimmer or classic component?

I'm assuming both are fairly similar? Glimmer would probably be okay on its own, but if there are notable differences, it might be good to note those.

• For classic syntax at least, I believe .get is supposed to Just Works™ here, should I be using that?

Probably worth mentioning in the docs, but we should probably prefer using regular ES getters.

I don't think it's particular dangerous, because at the end of the day it's just a weird quirk/fallout, but I'm not sure it's good to include it as an official API.
Got it. Sounds okay to leave out then.

[chancancode]

I'm writing some docs, but sounds like @chriskrycho wants to review this also

[maxfierke]

I'm writing some docs, but sounds like @chriskrycho wants to review this also

awesome! his suggestion of type tests would be super valuable too, if that's a thing

[chancancode]

Suggested change

	export type TaskGenerator<T> = Generator<any, T, unknown>;
	export type TaskGenerator<T> = Generator<any, T, any>;

Should the third parameter (the LHS of yield ..., i.e. what it "returns") be unknown or any?

Due to TS limitations, there is no way we can type that properly. What the yield "returns" is based on what you yielded, and that is just not possible to type. await has a similar problem but is hardcoded to work in the TS compiler.

The first parameter (the RHS of the yield ..., i.e. what you are allowed to yield to ember-concurrency) is typed to any because there is no real utility to limit it, it really can be anything (including nothing – you may just be yielding so ember-concurrency can pause to check for isDestroying and such).

The third parameter is typed to unknown, but I am not sure if that is too strict. In practice, it will force every consumer to check the value. If you are in the mood for it you may write an assertion:

class Foo {
  @task *myTask() {
    let foo /* unknown */ = yield someStringPromise;
    assert('foo should have resolved into a string!', typeof foo === 'string');
    // now foo is a string
  }
}

...that's fine when it's easy enough to check, but it's this is really checking for bugs in the library than in your code, and you would never have written that assertion there if not for TS. For more complex objects you may end up needing to write elaborate type assertion functions that are otherwise not needed. In practice, a lot of people is just going to end up writing this anyway, every single time they yield in a task:

class Foo {
  @task *myTask() {
    let foo: MyType = (yield someStringPromise) as any;
  }
}

If that's what everyone is going to do at the end of the day, it just seems a little silly not to type it as any in the first place. In practice, so long as you know the type of the RHS, there is very little reason not to trust e-c to call you with the correct thing on the LHS. If you are worried that you would have mistyped the LHS, you could do this:

type Resolved<T> = T extends PromiseLike<infer R> ? R : T;

class Foo {
  @task *myTask() {
    let foo: Resolved<typeof someStringPromise> = yield someStringPromise;
  }
}

That seems like a pretty good compromise between correctness and verbosity, and led me to thinking we should go with any instead of unknown here. Thoughts?

[chancancode]

Ah, I realized my mistake (I actually thought about it when I typed this), in practice it doesn't really matter what we typed here, because the types are never really used for inference in the position I was refereeing to above. Since it's inside the body of a generator function, TypeScript is just going to do whatever it normally does to generator functions without ever considering the type we have here. This type is only used when assigning a generator function to task(...), etc, as long as the type is wide enough that TS allows assigning any valid task function to positions where we take one as a TaskFunction<T>, it doesn't really matter.

[chriskrycho]

I think I'm on the same page given the constraints TS gives us. I would recommend we very carefully document this, however, because it's the kind of thing that could easily bite people by letting any leak in when they're unaware of it.

[chancancode]

I was wrong again. Here is how it comes up in practice (typing the return type of the generator function) and the ergonomic differences:

Playground

This doesn't look so bad since the types are just strings and the assertion is easy to write, but basically see my first comment for the tradeoffs.

[chancancode]

Talked to @chriskrycho on Discord, it seems like we are leaning towards using any here and documenting/recommending the Resolved type trick

[chancancode]

Right, the main question is do we want to force the consumer to do a check.

[chriskrycho]

As you noted, it's a very frustrating experience to have to when the type is actually knowable and local. I don't have a strong opinion here (and I normally very strongly favor unknown and forcing the check).

cc. also @dfreeman @jamescdavis

[chancancode]

Maybe I'll just go ahead and write a compiler plugin that transforms @task async methods into generators and no have to worry about this 😄

[chancancode]

Anddd done: https://github.com/chancancode/ember-concurrency-async

[chancancode]

I went with any in the end and documented it.

[chriskrycho]

This is a great set of types, and I don't have any objections to the types themselves as they stand! However, I'd like to very strongly recommend that we add a set of type tests, of the same sort I recently added to ember-modifier. The short of that:

• Install the expect-type library
• Create a type test file somewhere in the library; as long as it can import the add-on's types it should be good to go. (This may require adding a tsconfig.json file next to it to support it.)
• Write type assertions that make sure that the public API of the module is what it should be—the expectTypeOf type assertion will prevent accidentally widening to any, for example, if you give it a specific type.
• Add an ember-try config that runs against a supported set of TypeScript versions—see ember-modifier's ember-try.js and ember-try-typescript.js for a good example.
• Related to the ember-try config: define the supported TypeScript versions. We recommend supporting 3.6, 3.7, and 3.9 at present (all the versions released during the current or previous stable Ember LTS, excluding 3.8 because it had serious problems with Ember's types)

(I'm actually in the final stages of polishing up a Typed Ember RFC with guidance recommending exactly this strategy for all addons which ship types. Obviously that won't be able to force anyone's hand, any more than we can force anything for addons in the ecosystem in general, but we're hoping to get everyone on the same page so that we can have a set of solid guarantees around stable types in the ecosystem. I'll link that here when it's done, hopefully by the end of the day today.)

[chriskrycho]

As promised, Typed Ember RFC: Type Stability for Addons.

[chancancode]

Update: I have added the requested documentation, will work on the tests next. In the meantime please review what I wrote.

Rendered screenshot, for reference (the content will likely be outdated, but you can review the general formatting):

[chancancode]

I assumed we are going to bump minor version for this.

[chancancode]

I assumed this as well. I'm not sure what the version number would be, so I just said "latest". There is a 1.1 "alpha" that is pretty unrelated, so we may want to skip 1.1 to avoid confusion. Jumping to 1.2 would also match up with the e-c version number, a happy accident.

[chancancode]

(still relevant)

[chancancode]

Pushed a new commit to add the type tests, ember-try and CI config

[chriskrycho]

Thank you so much! I have this on my list for the week—it will likely be Tuesday or Wednesday before I have time to review it in detail!

[maxfierke]

Definite 👍 from me, but will defer merging until @chriskrycho is able to take another look. The docs look excellent!

[chriskrycho]

Had a handful of questions and suggestions. I will review the type tests tomorrow, but wanted to keep the conversation moving!

[chriskrycho]

What's the benefit of naming these Encapsulated* rather than just TaskDescriptor etc.? Particular in terms of the imports we're exposing with them, TaskDescriptor, TaskDescriptorArgs, and TaskDescriptorReturn all seem much better—and I don't see any overload with anything else here?

[chancancode]

Because the feature is called and documented as EncapsulatedTask. I considered just calling it EncapsulatedTask , but that's not quite right, because this is the thing you pass in (similar to the POJO you pass to EmberObject.extend(...)) to create a encapsulated task (technically, an encapsulated task property, which gets turned into an encapsulated task, which can be instantiated as encapsulated task instances). It just so happens that the current implementation of the task function normalizes away the "encapsulated-ness" of it so there aren't EncapsulatedTaskProperty, EncapsulatedTask and EncapsulatedTaskInstance exports, but there very well may be in the future.

As far as TaskDescriptor, that also sounds like too generic of a term, as it seems feasible that there will be a TaskDescriptor that is unrelated to EncapsulatedTask.

Ultimately, I question whether some of these APIs are still needed/still a good fit in the post-Octane world, but ultimately, it just doesn't seem like my decision to make or my job to speculate on the future direction too much in this PR, as my goal is just to document what exists today.

So EncapsulatedTaskDescriptorArgs seems like the most conservative, and accurate, if a bit verbose, option.

[chriskrycho]

This makes sense; thanks for the explanation! I had forgotten about the feature in question. 👍

[chriskrycho]

Really great work on these tests! I caught a number of things, but this is delightfully exhaustive and a fantastic model for the community. Thank you!

[chriskrycho]

Interesting—why is this needed? Is it not covered by the previous exclude, or is it just here to be extra explicit?

[chancancode]

I am not 100% sure that it did anything, tbh. However, e-c-d ships with its own ambient (not imported by anything) declaration that conflicts with the definitions here. When importing from e-c-d, it seems to cause that file to be "picked up" and causes type errors in the language server in VS Code, which appears to be fixed by this, or that I got lucky after adding it.

[chancancode]

In any case, I'll try to remember to come back and remove this once e-c-d has been updated

[chriskrycho]

Let's do this! Thanks again so much for the massive lift here!