add types definitions that also support ember-concurrency-decorators #355

NullVoxPopuli · 2020-04-29T00:06:54Z

After initial review

review Add Type Definitions #319, and address feedback from that PR
add some tests
- Octane App with ember-concurrency-decorators
- Classic App with classic function wrappers
de-namespace, as we can just export straight from this file, as TS already handles the namespacing because the index.d.ts will be in node-modules

Huge thanks to @buschtoens for the work done in #319

these types enable the following, given these helper functions:

import Task from 'ember-concurrency/task';

type ECTask<Args extends Array<any>, Return> = (
  ...args: Args
) => Generator<any /* potentially yielded types */, Return, unknown>;

export function taskFor<Args extends any[], Return = void>(generatorFn: ECTask<Args, Return>) {
  return (generatorFn as any) as Task<Args, Return>;
}

we can have inferred types from ember-concurrency-decorators!

maxfierke

I'm not a TypeScript user, so I don't have a very sophisticated understanding of TypeScript type definitions, so apologies for some fairly basic questions.

I also have some sort of broad questions:

Could you provide some details on what's different here versus Add Type Definitions #319?
- Does it address feedback raised there?
Are there TypeScript version considerations?
Is there a way to test/validate these (something that could be thrown in CI)?

The latter would make this more likely to be merged.

maxfierke · 2020-04-29T02:22:19Z

index.d.ts

+    restartable(): TaskDecorator;
+    maxConcurrency(num: number): TaskDecorator;
+    enqueue(): TaskDecorator;
+    withTestWaiter(): PropertyDecorator;


this is defined by ember-test-waiters. perhaps this should live there? (is that a possible thing?)

maybe? but should https://github.com/bendemboski/ember-concurrency-test-waiter get pulled in to this addon since it's a useful tool to help in testing with tasks?

maxfierke · 2020-04-29T02:22:37Z

index.d.ts

+    restartable(): TaskDecorator;
+    maxConcurrency(num: number): TaskDecorator;
+    enqueue(): TaskDecorator;
+    withTestWaiter(): PropertyDecorator;


maxfierke · 2020-04-29T02:32:04Z

index.d.ts

+  export enum TaskState {
+    RUNNING = 'running',
+    QUEUED = 'queued',
+    IDLE = 'idle',
+  }


My understanding based on some feedback in #319, is that we should not export this in a type definition, since there is no such enum available to import.

maxfierke · 2020-04-29T02:32:27Z

index.d.ts

+  export enum TaskInstanceState {
+    /** task instance was canceled before it started */
+    DROPPED = 'dopped',
+
+    /** task instance was canceled before it could finish */
+    CANCELED = 'canceled',
+
+    /** task instance ran to completion (even if an exception was thrown) */
+    FINISHED = 'finished',
+
+    /** task instance is currently running (returns true even if is paused on a yielded promise) */
+    RUNNING = 'running',
+
+    /** task instance hasn't begun running yet (usually because the task is using the `TaskProperty#enqueue` task modifier) */
+    WAITING = 'waiting',
+  }


same deal here, re: exporting of an enum

yeah, these probably don't need to be exported -- this is mostly copy paste from long ago

maxfierke · 2020-04-29T02:35:48Z

index.d.ts

+  };
+
+  export function task(
+    generator: Function


should this be a GeneratorFunction (or w/e the TypeScript-equivalent type name is)?

depends on if any type inference could be gained from doing so, like how I'm getting inference with decorators

maxfierke · 2020-04-29T02:37:06Z

index.d.ts

+    /**
+     * The most recently started task instance.
+     */
+    readonly last: TaskInstance<PerformArgs, PerformReturn>;


this (and the other last* properties) can also be null. Do these need to be denoted as nullable?

oh! yes, you're absolutely right. (though is it null or undefined?)

NullVoxPopuli · 2020-04-29T03:19:24Z

Could you provide some details on what's different here versus #319?

the main difference is that I've added some generics that help with type inference with decorators (see screenshots in original comment)

Does it address feedback raised there?

nope, I need to look at that

Are there TypeScript version considerations?

no clue, I'm only using 3.7.x in my apps

Is there a way to test/validate these (something that could be thrown in CI)?

yea - we could build this in to the repo with full ember apps that aren't the dummy app, and try to use all of the APIs and make sure there are no type errors?

chancancode · 2020-05-29T00:09:43Z

index.d.ts

+  };
+  export function timeout(wait: number): Promise<void>;
+
+  export function didCancel(error: Error): boolean;


This should probably be export function didCancel(reason: unknown): reason is Error;. In a promise callback, I don't think you necessarily know the reject reason object is an Error, and so if you require this parameter to be Error TypeScript might not let you pass it.

maxfierke · 2020-06-16T20:09:35Z

Thanks so much for your work on this! However, we're closing this in favor of #357, which has been merged and will be part of 1.2.0

add types for ember-concurrency

06239df

NullVoxPopuli changed the title ~~add types for ember-concurrency~~ add types definitions that also support ember-concurrency-decorators Apr 29, 2020

maxfierke reviewed Apr 29, 2020

View reviewed changes

chancancode reviewed May 29, 2020

View reviewed changes

chancancode mentioned this pull request Jun 2, 2020

Add type definition based on API docs #357

Merged

maxfierke closed this Jun 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add types definitions that also support ember-concurrency-decorators #355

add types definitions that also support ember-concurrency-decorators #355

NullVoxPopuli commented Apr 29, 2020 •

edited

Loading

maxfierke left a comment

maxfierke Apr 29, 2020

NullVoxPopuli Apr 29, 2020

maxfierke Apr 29, 2020

maxfierke Apr 29, 2020

maxfierke Apr 29, 2020

NullVoxPopuli Apr 29, 2020

maxfierke Apr 29, 2020

NullVoxPopuli Apr 29, 2020

maxfierke Apr 29, 2020

NullVoxPopuli Apr 29, 2020

NullVoxPopuli commented Apr 29, 2020

chancancode May 29, 2020

maxfierke commented Jun 16, 2020

add types definitions that also support ember-concurrency-decorators #355

add types definitions that also support ember-concurrency-decorators #355

Conversation

NullVoxPopuli commented Apr 29, 2020 • edited Loading

maxfierke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Apr 29, 2020

Choose a reason for hiding this comment

maxfierke commented Jun 16, 2020

NullVoxPopuli commented Apr 29, 2020 •

edited

Loading