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’ll 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
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 139 additions & 0 deletions addon/ember-concurrency.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
declare module 'ember-concurrency' {
import EmberObject from '@ember/object';
import {
UnwrapComputedPropertyGetter,
UnwrapComputedPropertyGetters
} from '@ember/object/-private/types';

import RSVP from 'rsvp';

// Lifted from @types/ember__object/observable.d.ts
interface Getter {
/**
* Retrieves the value of a property from the object.
*/
get<K extends keyof this>(key: K): UnwrapComputedPropertyGetter<this[K]>;
/**
* To get the values of multiple properties at once, call `getProperties`
* with a list of strings or an array:
*/
getProperties<K extends keyof this>(
list: K[]
): Pick<UnwrapComputedPropertyGetters<this>, K>;
getProperties<K extends keyof this>(
...list: K[]
): Pick<UnwrapComputedPropertyGetters<this>, K>;
}

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

export const all: typeof Promise.all;
export const allSettled: typeof RSVP.allSettled;
export const hash: typeof RSVP.hash;
export const race: typeof Promise.race;

export function timeout(ms: number): Promise<void>;

export function waitForEvent(
object: EmberObject | EventTarget,
eventName: string
): Promise<Event>;

export function waitForProperty<T extends object, K extends keyof T>(
object: T,
key: K,
callbackOrValue: (value: T[K]) => boolean
): Promise<void>;

export function waitForQueue(queueName: string): Promise<void>;

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

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.

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

export function taskGroup(): TaskGroupProperty;

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

export interface TaskProperty extends CommonTaskProperty {
evented: () => TaskProperty;
debug: () => TaskProperty;
on: (eventName: string) => TaskProperty;
}

// eslint-disable-next-line @typescript-eslint/no-empty-interface
export interface TaskGroupProperty extends CommonTaskProperty {}

// Based on https://github.com/CenterForOpenScience/ember-osf-web/blob/7933316efae805e00723789809bdeb58a96a286a/types/ember-concurrency/index.d.ts

export enum TaskInstanceState {
Dropped = 'dropped',
Canceled = 'canceled',
Finished = 'finished',
Running = 'running',
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.

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>;
}

export enum TaskState {
Running = 'running',
Queued = 'queued',
Idle = 'idle'
}

export interface Task<Args extends any[], T> extends Getter {
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;
}
}
46 changes: 40 additions & 6 deletions addon/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ import {
task as createTaskProperty,
taskGroup as createTaskGroupProperty,
TaskProperty,
TaskGroupProperty
TaskGroupProperty,
Task,
GeneratorFn
} from 'ember-concurrency';

export { default as lastValue } from './last-value';
Expand Down Expand Up @@ -86,7 +88,7 @@ function createTaskFromDescriptor(desc: DecoratorDescriptor) {
(typeof value === 'object' && typeof value.perform === 'function')
);

return createTaskProperty(value);
return (createTaskProperty(value) as unknown) as TaskProperty;
}

/**
Expand All @@ -110,14 +112,15 @@ function createTaskGroupFromDescriptor(_desc: DecoratorDescriptor) {
*/
function applyOptions(
options: TaskGroupOptions,
task: TaskGroupProperty
taskProperty: TaskGroupProperty
): TaskGroupProperty & Decorator;
function applyOptions(
options: TaskOptions,
task: TaskProperty
taskProperty: TaskProperty
): TaskProperty & Decorator {
return Object.entries(options).reduce(
(
// eslint-disable-next-line no-shadow
taskProperty,
[key, value]: [
keyof typeof options,
Expand All @@ -135,7 +138,7 @@ function applyOptions(
value
);
},
task
taskProperty
// The CP decorator gets executed in `createDecorator`
) as TaskProperty & Decorator;
}
Expand Down Expand Up @@ -192,7 +195,38 @@ const createDecorator = (
* @param {object?} [options={}]
* @return {TaskProperty}
*/
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

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

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—one extra generator function allocation, it looks like?—that 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.

taskFn: GeneratorFn<Args, R>
): Task<Args, Exclude<R, Promise<any>>>;
export function task<Args extends any[], R>(encapsulatedTask: {
perform: GeneratorFn<Args, R>;
}): Task<Args, Exclude<R, Promise<any>>>;
export function task(options: TaskOptions): PropertyDecorator;
export function task(
target: Record<string, any>,
propertyKey: string | symbol
): void;
export function task<Args extends any[], R>(
...args:
| [GeneratorFn<Args, R>]
| [{ perform: GeneratorFn<Args, R> }]
| [TaskOptions]
| [Record<string, any>, string | symbol]
): Task<Args, Exclude<R, Promise<any>>> | PropertyDecorator | void {
const [firstParam] = args;
if (
typeof firstParam === 'function' ||
(typeof firstParam === 'object' &&
// @ts-ignore
typeof firstParam.perform === 'function')
)
// @ts-ignore
return firstParam;
// @ts-ignore
return taskDecorator(...args);
}

/**
* Turns the decorated generator function into a task and applies the
Expand Down
83 changes: 54 additions & 29 deletions tests/unit/decorators-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,60 +19,80 @@ module('Unit | decorators', function() {

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

@restartableTask
a = function*() {
a = task(function*() {
yield;
return 456;
};
});

@keepLatestTask
b = function*() {
b = task(function*() {
yield;
return 789;
};
});

@dropTask
c = function*() {
c = task(function*() {
yield;
return 12;
};
});

@enqueueTask
d = function*() {
d = task(function*() {
yield;
return 34;
};
});
}

let subject!: TestSubject;
run(() => {
subject = TestSubject.create();
// @ts-ignore
subject.get('doStuff').perform();
// @ts-ignore
subject.get('a').perform();
// @ts-ignore
subject.get('b').perform();
// @ts-ignore
subject.get('c').perform();
// @ts-ignore
subject.get('d').perform();
});
// @ts-ignore
assert.equal(subject.get('doStuff.last.value'), 123);
// @ts-ignore
assert.equal(subject.get('a.last.value'), 456);
// @ts-ignore
assert.equal(subject.get('b.last.value'), 789);
// @ts-ignore
assert.equal(subject.get('c.last.value'), 12);
// @ts-ignore
assert.equal(subject.get('d.last.value'), 34);
assert.equal(
subject
.get('doStuff')
.get('last')!
.get('value'),
123
);
assert.equal(
subject
.get('a')
.get('last')!
.get('value'),
456
);
assert.equal(
subject
.get('b')
.get('last')!
.get('value'),
789
);
assert.equal(
subject
.get('c')
.get('last')!
.get('value'),
12
);
assert.equal(
subject
.get('d')
.get('last')!
.get('value'),
34
);
});

// This has actually never worked.
Expand All @@ -81,21 +101,26 @@ module('Unit | decorators', function() {

class TestSubject extends EmberObject {
@task
encapsulated = {
encapsulated = task({
privateState: 56,
*perform() {
yield;
return this.privateState;
}
};
});
}

let subject!: TestSubject;
run(() => {
subject = TestSubject.create();
subject.get('encapsulated').perform();
});
// @ts-ignore
assert.equal(subject.get('encapsulated.last.value'), 56);
assert.equal(
subject
.get('encapsulated')
.get('last')
.get('value'),
56
);
});
});
Loading