Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: PoC for TypeScript type support #56

Closed
wants to merge 7 commits into from

Conversation

buschtoens
Copy link
Collaborator

@buschtoens buschtoens commented Aug 6, 2019

馃敭

This all is super messy and will require refactoring, but it proves the concept right. 馃帀

The only remaining issue is the this context in encapsulated tasks, but I am happy to let that slide now and revisit it later.

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

class TestSubject extends EmberObject {
  @task
  doStuff = task(function*() {
    yield;
    return 123;
  });

  @task
  encapsulated = task({
    privateState: 56,
    *perform() {
      yield;
      return this.privateState; // <- broken
    }
  });
}

const subject = TestSubject.create();
const value = await this.doStuff.perform(); // => 123

Closes #30. Kinda closes #41.

Kinda closes buschtoens/ember-concurrency-typescript#6 and makes the whole project obsolete. 馃槃

Waiting = 'waiting'
}

export interface TaskInstance<T> extends PromiseLike<T>, Getter {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to tweak this to be a union type where TaskInstanceState and the various booleans are all always guaranteed/required to be the same.

interface TaskInstanceBase<T> extends PromiseLike<T>, Getter {
    readonly hasStarted: boolean;
    readonly isCanceled: boolean;
    readonly isDropped: boolean;
    readonly isFinished: boolean;
    readonly isRunning: boolean;
    readonly isSuccessful: boolean;
    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>;
}

interface InstanceDropped {
    readonly state: TaskInstanceState.Dropped;
    hasStarted: true; // ?
    isCanceled: true; // ?
    isDropped: true;
    isError: false; // ?
    isFinished: true; // ?
    isRunning: false; // ?
    isSuccessful: false; // ?
}

interface InstanceCanceled {
    readonly state: TaskInstanceState.Canceled;
    hasStarted: true; // ?
    isCanceled: true;
    isDropped: false; // ?
    isError: false; // ?
    isFinished: true; // ?
    isRunning: false; // ?
    isSuccessful: false; // ?
};

// etc.

interface InstanceError {
    isError: true;
    error: unknown;
    hasStarted: true; // ?
    isCanceled: false; // ?
    isDropped: false; // ?
    isFinished: true;
    isRunning: false;
    isSuccessful: false;
}

interface Success {
    isError: false;
    error: undefined;
    hasStarted: true; // ?
    isCanceled: false; // ?
    isDropped: false; // ?
    isError: false;
    isFinished: true;
    isRunning: false;
    isSuccessful: true;
}

export type TaskInstance<T> = TaskInstanceBase<T> & (
    | InstanceDropped
    | InstanceCanceled
    | InstanceFinished
    | InstanceRunning
    | InstanceWaiting
    | InstanceError
    | InstanceSuccess
);

The // ?s are for all the places where I don't remember from Ember Concurrency's docs what the actual behavior is 鈥 some of those may be boolean always instead of a const type like true or false.

export const task = createDecorator(createTaskFromDescriptor);
const taskDecorator = createDecorator(createTaskFromDescriptor);

export function task<Args extends any[], R>(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use docs, of course!

I think I'm following this; why does it require both @task and = task, though? It seems like it would incur extra overhead?

Copy link
Collaborator Author

@buschtoens buschtoens Aug 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@task is what actually makes this a task at runtime.

= task(...) is what makes this look like a task to TypeScript.

Unfortunately, either cannot do what the other one does, so we have to use this awkward double.

Copy link
Collaborator Author

@buschtoens buschtoens Aug 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically = task(...) can do @task's job as well, using a babel transform. This is what I explored in ember-concurrency-typescript. Unfortunately, I then hit buschtoens/ember-concurrency-typescript#6 with it, which made the whole thing kinda moot.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, that's what I thought; how do we make it not incur runtime costs? Or is the judgment simply that they're small enough鈥攐ne extra generator function allocation, it looks like?鈥攖hat it doesn't really matter?

Copy link
Collaborator Author

@buschtoens buschtoens Aug 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not allocating an extra generator function, luckily! :)

= task(function*() {}) / = task({ *perform() {} }) trigger this code branch:

if (
typeof firstParam === 'function' ||
(typeof firstParam === 'object' &&
// @ts-ignore
typeof firstParam.perform === 'function')
)
// @ts-ignore
return firstParam;

This means that in this case, = task(...) just passes through the generator function. So effectively, the following is equivalent:

@task
foo = task(function*() {});

// is equivalent to

@task
foo = function*() {};

This doesn't mean though, that we can't optimize this further with a Babel transform. 鈿★笍 馃槃

@task
foo = task(function*() {});

// may be turned into

@task // from `ember-concurrency-decorators`
*foo() {}

// or even the following to avoid _any_ extra e-c-d runtime

@task(function*() {}) foo; // from `ember-concurrency`

Copy link
Collaborator Author

@buschtoens buschtoens Aug 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect rambling here

I need to correct myself. The following still does allocate a new generator function for every class instance:

@task
foo = function*() {}; // takes the `desc.initializer` path

While this one does not allocate a new generator function for every class instance:

@task
foo*() {} // takes the `desc.value` path

function extractValue(desc: DecoratorDescriptor): any {
if (typeof desc.initializer === 'function') {
return desc.initializer.call(null);
}
if (typeof desc.get === 'function') {
return desc.get.call(null);
}
if (desc.value) {
return desc.value;
}
}

I think. tbh I'm not 馃挴sure right now, when in the life cycle extractValue is called. 馃槄

Edit: No, looks like desc.initializer is only ever called once. Before the class is actually instantiated, which is also why it's called on null.


export function task<Args extends any[], R>(
taskFn: GeneratorFn<Args, R>
): Task<Args, Exclude<R, Promise<any>>>;
Copy link
Collaborator Author

@buschtoens buschtoens Aug 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm so jazzed about that.

We should talk about TS version support at some point as well; it's probably fine to start with 3.6, given that and depending on when this lands and TS 3.6 lands, but the Typed Ember team is also working on hashing out a good ecosystem-wide versioning strategy as well to deal with the semi-regularity (and non-semver-compatible) breaking changes. Our current actual practice is "at least two most recent versions" but that's not going to work long-term.

// @TODO: this does not work
perform: GeneratorFn<E, Args, R>;
}
>(encapsulatedTask: E): Task<Args, Exclude<R, Promise<any>>>;
Copy link
Collaborator Author

@buschtoens buschtoens Aug 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this self-referential trickery does not work. 鈽癸笍

Copy link
Collaborator Author

@buschtoens buschtoens Aug 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can also be a blessing in disguise though!

class Foo {
  @task
  encapsulated = task(class {
    privateState = 123;
    *perform() {
      return this.privateState;
    }
  });
}

This would solve #37.

Copy link
Collaborator Author

@buschtoens buschtoens Aug 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @spencer516 FYI 馃槉

@buschtoens
Copy link
Collaborator Author

@buschtoens buschtoens commented Aug 13, 2019

I published this as a pre-release v1.1.0-alpha.1 so you can give this a try in real-world apps.

@buschtoens
Copy link
Collaborator Author

@buschtoens buschtoens commented Aug 21, 2019

Do any of you TypeScript whizzes know a way, preferably not requiring users of ember-concurrency-decorators to change their tsconfig.json, that would allow ember-concurrency.d.ts to take effect?

I thought putting it inside addon instead of types would already do the trick, but the types are unavailable, when I add ember-concurrency-decorators@alpha as a dependency.

$ tsc
../../../node_modules/ember-concurrency-decorators/index.d.ts:1:49
  error TS7016: Could not find a declaration file for module 'ember-concurrency'.

  '/Users/<snipsnip>/code/clark/node_modules/ember-concurrency/index.js'
  implicitly has an 'any' type.

  Try `npm install @types/ember-concurrency` if it exists or add a new
  declaration (.d.ts) file containing `declare module 'ember-concurrency';`

1 import { TaskProperty, Task, GeneratorFn } from 'ember-concurrency';

@jamescdavis
Copy link
Contributor

@jamescdavis jamescdavis commented Aug 22, 2019

I think this will work.

@buschtoens
Copy link
Collaborator Author

@buschtoens buschtoens commented Sep 17, 2019

Short update: We've been using this in production with great success for a few weeks now. The only remaining issue is providing the types to the consuming host application, which I still need to look into (#56 (comment)).

buschtoens added a commit to buschtoens/ember-concurrency that referenced this issue Sep 18, 2019
@buschtoens
Copy link
Collaborator Author

@buschtoens buschtoens commented Sep 18, 2019

I opened machty/ember-concurrency#319 to upstream the types from this PR.

@lolmaus
Copy link

@lolmaus lolmaus commented Oct 12, 2019

Can you guys please explain why you need this over using plain task CP macro from ember-concurrency as a decorator:

@(task(function * () {}).restartable())
myTask!;

I believe, the code above:

  1. Instantiates a task properly.
  2. Lets the TypeScript know the correct type.
  3. Uses vanilla ember-concurrency, does not require any extra dependencies.
  4. Works already, does not require any extra work to be done.

You can even provide a custom return type:

myTask!: Task<Product>;

Why the above does not work for you and why do you want to change it for the code below? What's the gain?

@restartableTask
myTask = task(function * () {});

@chriskrycho
Copy link

@chriskrycho chriskrycho commented Oct 12, 2019

Thanks for the question, @lolmaus 鈥 this is a good chance to explain for other folks as well.

Can you guys please explain why you need this over using plain task CP macro from ember-concurrency as a decorator:

@(task(function * () {}).restartable())
myTask!;

I believe, the code above:

  1. Instantiates a task properly.
  2. Lets the TypeScript know the correct type.
  3. Uses vanilla ember-concurrency, does not require any extra dependencies.
  4. Works already, does not require any extra work to be done.

Of these, (1), (3), and (4) are correct, but (3) and (4) are correct only in a trivial sense because (2) is quite wrong. There is no type at all here for TypeScript: it鈥檚 implicitly typed as any. TypeScript has never supported changing the type of a class property with a decorator, and will certainly not do so at least until the decorators proposal reaches Stage 3. (They may never do so.)

What this means is that you have to specify the Task type on the property, as in your suggestion:

You can even provide a custom return type:

myTask!: Task<Product>;

Why the above does not work for you and why do you want to change it for the code below? What's the gain?

@restartableTask
myTask = task(function * () {});

The problem with doing as you suggest is that there鈥檚 a total decoupling between the type declaration and the actual function supplied. You can easily make a mistake even when first writing the task; more problematically, it鈥檒l keep compiling just fine if you later need to make the yielded type of the Task be Product | null鈥 and then you鈥檙e getting a Cannot access property price on null error at runtime! 馃槩

What we actually want is to be able to have TypeScript check our code without our doing extra work when writing app code鈥攖hat is, to do a bit of extra work on the library end so that users can just write fairly normal code and have the types always get checked and never get out of sync. That鈥檚 what this proposal does!

@lolmaus
Copy link

@lolmaus lolmaus commented Oct 14, 2019

Thank you for clarifications, @chriskrycho.

@BryanCrotaz
Copy link

@BryanCrotaz BryanCrotaz commented Mar 25, 2020

any progress on this? ember-concurrency + typescript is the source of almost all my build errors

@jamescdavis
Copy link
Contributor

@jamescdavis jamescdavis commented Jul 8, 2020

@maxfierke @machty @chancancode IMHO we can now close this as https://github.com/chancancode/ember-concurrency-ts#alternate-usage-of-taskfor now provides the same functionality and is a migration path for anyone who jumped on v1.1.0-alpha.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants