machty / ember-concurrency-decorators

Task decorators for ember-concurrency 1.x (merged into ember-concurrency 2.0)
MIT License
70 stars 25 forks source link

TypeScript : "Property 'perform' does not exist..." on Task #30

Closed theseyi closed 3 years ago

theseyi commented 5 years ago

The following causes typescript to raise the type error at the bottom:

import Component from '@ember/component';
import { task } from 'ember-concurrency-decorators';

export default class ExampleComponent extends Component {
  @task
  *doStuff() {
  }

  executeTheTask() {
    this.doStuff.perform();
  }
}
Error:(75, 52) TS2339: Property 'perform' does not exist on type '() => IterableIterator<any>'.
buschtoens commented 5 years ago

That warning is "correct". Unfortunately decorators still cannot change the type signature of the thing that they decorate. The TypeScript will only further develop decorators support, once the proposal for decorators hit stage 3. But they are aware and have it on their road map.

I have gone through numerous ideas for workarounds in the #e-typescript and they all fall flat on their face in one they or another. Another big problem is that the type definition for generator functions loses some information, as the types of all yielded values and the return value are merged.

The "best" solution would require significant syntactical overhead to force-feed TypeScript:

import Component from '@ember/component';
import { task, Task } from 'ember-concurrency-decorators';

export default class ExampleComponent extends Component {
  @task
  *doStuff() {
  }

  executeTheTask() {
    ((this.doStuff as unknown) as Task).perform();
  }
}

The above code assumes a Task type to be exported by this addon.

buschtoens commented 5 years ago

Unfortunately it is currently impossible to extract the return value of a generator function: https://github.com/Microsoft/TypeScript/issues/2983

But I have another idea to make this addon somewhat more type-safe:

import Controller from '@ember/controller';
import { task, taskGroup, TaskGroup, Task, asTask } from 'ember-concurrency-decorators';

export class Foo extends Controller {
  @taskGroup saveTaskGroup!: TaskGroup;

  @task
  *someTask(someArg: number) {
    yield 'foo';
    return false;
  }

  @task
  encapsulated = {
    *perform(someArg: number) {
      yield 'foo';
      return false;
    }
  };

  exec() {
    const foo = asTask(this.someTask).perform<boolean>(3);
    const bar = asTask(this.encapsulated).perform<boolean>(3);

    this.saveTaskGroup.cancelAll();
  }
}

Some explanation is due:

Since task groups don't get assigned a value, we can just add a type and non-null assertion operator to promise TypeScript, that this property is gonna be set.

@taskGroup saveTaskGroup!: TaskGroup;

This allows plain and simple access without any further trickery like you would normally do:

this.saveTaskGroup.cancelAll();

For tasks it is not as pretty. I took some inspiration from idx, which is a clever combination of type definitions and a babel transform, that makes it a zero-cost abstraction.

const foo = asTask(this.someTask).perform<boolean>(3); // => Promise<boolean>
const bar = asTask(this.encapsulated).perform<boolean>(3); // => Promise<boolean>

asTask would do something similar. By passing the task (this.task / this.encapsulated) into a virtual util function of which we can control the input types, we can avoid the cast to unknown as suggested in https://github.com/machty/ember-concurrency-decorators/issues/30#issuecomment-454952242. Additionally, this also means that we do not lose the type information about the method, which includes the argument types.

asTask will convert any generator function or encapsulated task object into an actual Task with a perform method whose arguments are the same as the ones from the original generator function.

Unfortunately, we cannot infer the return type of a generator function (https://github.com/Microsoft/TypeScript/issues/2983). Even more unfortunately we also cannot specify the return type as a generic type parameter to the Task itself, like:

const foo = asTask<boolean>(this.someTask).perform(3);
const bar = asTask<boolean>(this.encapsulated).perform(3);

This would break the type inferring for the arguments, because of https://github.com/Microsoft/TypeScript/pull/26349. 🙈

I have the type side of things working on a branch and will experiment a bit with it, and see whether I can optimize this further somehow.

Writing the babel transform should be fairly easy.

Apropos, why do we need the babel transform in the first place?

If asTask was an actual runtime util function, the task that would be passed to it would lose its this context. The transform will just strip asTask and will convert the above code to basically just this:

const foo = this.someTask.perform(3);
const bar = this.encapsulated.perform(3);
buschtoens commented 5 years ago

If we assume, that users only yield Promises, we can actually infer the return type, because the return value cannot be a Promise, since ember-concurrency would await and unwrap it.

If I add the following (unfinished) types:

export function asTask<Args extends any[], R>(
  task: GeneratorFn<Args, R>
): Task<Args, Exclude<R, Promise<any>>>;

export function asTask<Args extends any[], R>(task: {
  perform: GeneratorFn<Args, R>;
}): Task<Args, Exclude<R, Promise<any>>>;

export interface Task<Args extends any[], R> {
  perform(...args: Args): Promise<R>;
  lastSuccessful?: {
    value: R;
  };
  // ...
}

export interface TaskGroup {
  cancelAll(): void;
  // ...
}

type GeneratorFn<Args extends any[] = any[], R = any> = (
  ...args: Args
) => IterableIterator<R>;

The following works:

import Controller from '@ember/controller';
import { task, taskGroup, TaskGroup, Task, asTask } from 'ember-concurrency-decorators';

export class Foo extends Controller {
  @taskGroup saveTaskGroup!: TaskGroup;

  @task
  *someTask(someArg: number) {
    yield Promise.resolve('foo');
    return false;
  }

  @task
  encapsulated = {
    *perform(someArg: number) {
      yield Promise.resolve('foo');
      return false;
    }
  };

  exec() {
    const foo = asTask(this.someTask).perform(3); // => Promise<false>
    const bar = asTask(this.encapsulated).perform(3); // => Promise<false>

    this.saveTaskGroup.cancelAll();
  }
}

Big but: if users yield a non-Promise value, it would be part of the return type.

class {
  @task
  *someTask(someArg: number) {
    yield Promise.resolve('foo');
    yield 'bar';
    return false;
  }
  exec() {
    const foo = asTask(this.someTask).perform(3); // => Promise<'bar' | false>
  }
}

IMO yielding a something that is not a Promise (assuming everything is typed properly here) does not make sense and users would be nudged to remove the incorrect yield or fix the type of it.

In worst case scenario, they could still cast the return value without as unknown since the return type will be a super set of the actual return type.

buschtoens commented 5 years ago

@jamescdavis suggested another great idea on Discord (thread). He wrote types that allow the following:

class Foo {
  myTask = task(function*(this: Foo) {
    yield Promise.resolve('foo');
    return 'bar';
  });

  exec() {
    this.myTask.perform();
  }
}

AFAICT this has a problem though. It should cause https://github.com/machty/ember-concurrency/issues/271.

But if you combine this with the babel transform approach, we could transform all occurrences of identifier = task(...) to something that is similar to what the @task decorator does and defines the task on the prototype.

The massive advantage of this approach is, that you don't have to wrap your task in asTask() whenever you want to access it.

On the long run, when TS finally supports the new decorators spec, we'll probably want to converge on the @task decorator solution, since the syntax looks nicer IMO and also would not need any transpilation then.

I think I can easily implement both solutions in parallel. We can also provide codemods to switch between the two and also to strip asTask(), when TypeScript lands decorator support.

peabnuts123 commented 5 years ago

First of all I want to say I commend everybody's effort in making this addon in an attempt to bridge some gaps in the ecosystem right now. But to be honest, this issue makes this addon a bit useless. As it stands, you can only use this addon if you are using ES6 classes with JavaScript. I would say people are mostly adopting native classes because of TypeScript - which this addon essentially doesn't support right now (in no small part due to TypeScript itself). Coupled with the fact that the main addon doesn't work at all using native-classes, this leaves a pretty decent amount of projects that essentially cannot use ember-concurrency at-all. Is there some kind of workaround that can be developed to declare the tasks in a different way for the interim? Say, a utility function that takes a generator and returns an instance of Task or something (roughly suggested above), but still requiring the decorator? In the future we could then just remove that function. Or perhaps a type definition that we can explicitly define our Task properties as? I understand the limitations of inferring types from generators and that's a perfectly legitimate issue to be resolved upstream (FWIW: It's not a safe assumption to assume people will only yield Promises. It's pretty common to just return a value from an async function if you don't need to do any async work, as the async/await standard is expected to deal with that).

For what it's worth, you can get-by for now by just laundering your types through any:

  init() {
    super.init();
    this.myTask.perform();
  }

  // @NOTE Launder types here by explicitly typing `myTask` as `any`
  @task
  myTask: any = function*() {
    // ...
  };
buschtoens commented 5 years ago

First of all I want to say I commend everybody's effort in making this addon in an attempt to bridge some gaps in the ecosystem right now.

Thanks 😊

But to be honest, this issue makes this addon a bit useless. As it stands, you can only use this addon if you are using ES6 classes with JavaScript. I would say people are mostly adopting native classes because of TypeScript - which this addon essentially doesn't support right now (in no small part due to TypeScript itself).

I'm totally with you!

Is there some kind of workaround that can be developed to declare the tasks in a different way for the interim? Say, a utility function that takes a generator and returns an instance of Task or something (roughly suggested above), but still requiring the decorator?

Yes, I do think so! I haven't tested this, but I think this would be the right direction.


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

type GeneratorFn<Args extends any[] = any[], R = any> = (
  ...args: Args
) => IterableIterator<R>;

function asTask<Args extends any[], R>(
  task: GeneratorFn<Args, R>
): Task<Args, Exclude<R, Promise<any>>> {
  // @ts-ignore
  return task;
}

interface Task<Args extends any[], R> {
  perform(...args: Args): Promise<R>;
  lastSuccessful?: {
    value: R;
  };
  // ...
}

class Foo {
  bar = 1337;

  @task
  myTask = asTask(function*(this: Foo) {
    yield timeout(500);
    return this.bar;
  });
}
buschtoens commented 5 years ago

I made a PoC in #50.

jamescdavis commented 4 years ago

This is now solved by https://github.com/chancancode/ember-concurrency-ts

https://github.com/chancancode/ember-concurrency-ts#alternate-usage-of-taskfor provides the same functionality as #56