Add Type Definitions #319
Add Type Definitions #319
Conversation
|
I at least can give this a close look on Friday (have time allocated for miscellaneous open source woke then!). |
| */ | ||
| cancel(): void; | ||
|
|
||
| catch(): RSVP.Promise<unknown>; |
There was a problem hiding this comment.
Are there any downsides to using the native Promise here?
| catch(): RSVP.Promise<unknown>; | |
| catch(): Promise<unknown>; |
There was a problem hiding this comment.
Yes: it’s not what the library actually returns! RSVP is a compatible superset type-wise, so you can test it as a normal Promise in your own codebase as a consumer, but if you need to take advantage of its being RSVP, you can’t if you make this change.
There was a problem hiding this comment.
I haven't had a chance to look at this thoroughly, but a good start would be to add a good number of test cases w/ dtslint.
| import { | ||
| UnwrapComputedPropertyGetter, | ||
| UnwrapComputedPropertyGetters | ||
| } from '@ember/object/-private/types'; |
There was a problem hiding this comment.
These private types change all the time, and are often in need of adjustment to respond to breaking TS compiler changes. Please do not consume these directly
| * Describes the state that the task instance is in. | ||
| * Can be used for debugging, or potentially driving some UI state. | ||
| */ | ||
| export enum TaskInstanceState { |
There was a problem hiding this comment.
I'd avoid creating the TaskInstance and TaskState enums, since they suggest that values (not just types) can be imported from ember-concurrency. If it's necessary to make some utility types available to consumers, that should probably be done through a ghost module to avoid clashing with anything that might be exported as a value from the underlying library in the future
There was a problem hiding this comment.
There was a problem hiding this comment.
haha. I suspect the quality bar may be different different between Chris' personal gist and merging something in as official typings.
There was a problem hiding this comment.
That’s just crazy talk. 😆 I agree here. Much as the enums are nice, if they can’t actually be imported we shouldn’t use them. Instead, we should just use literal types.
| /** | ||
| * Retrieves the value of a property from the object. | ||
| */ | ||
| get<K extends keyof this>(key: K): UnwrapComputedPropertyGetter<this[K]>; |
There was a problem hiding this comment.
This is going to break for cases like this.get('foo.bar'). Not all values that can be passed into this function are keyof this
There was a problem hiding this comment.
We have the same in DT: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/de12907d79777d50ce08036827da7425853abefd/types/ember__object/observable.d.ts#L12
Don't we just say getting in this way is not supported when using TS?
There was a problem hiding this comment.
Don't we just say getting in this way is not supported when using TS?
This is the current state of the types, given that we don't have the ability to use unknown as long as we continue to support TS 2.8.
Something like this (available in TS 3.0+) would support Ember's public API fully, without compromising type safety
interface Getter {
get<K extends keyof this>(key: K): this[K];
get(key: string): unknown;
}
interface Foo extends Getter {
blue: string[];
red: {
green: string;
}
}
const foo: Foo = {} as any;
foo.get('blue'); // string[];
foo.get('red.green'); // unknownThere was a problem hiding this comment.
We should note that as a point for discussion Soon(tm). 3.0 was a long time ago and the official support policy we decided on currently only commits us to 3.5 and 3.6, I believe. We’ve intentionally supported a wider range, but it’s been quite some time and it’s worth revisiting what we support (outside this PR!).
| getProperties<K extends keyof this>( | ||
| ...list: K[] | ||
| ): Pick<UnwrapComputedPropertyGetters<this>, K>; | ||
| } |
There was a problem hiding this comment.
Is the goal here to prevent (via types) setting values on tasks and task instances? Maybe we should just extend EmberObject (since tasks and task instances are)?
| export function waitForQueue(queueName: string): Promise<void>; | ||
|
|
||
| export function task<Args extends any[], R>( | ||
| taskFn: GeneratorFn<unknown, Args, R> |
There was a problem hiding this comment.
Typing the task function is awesome for type safety, but, unfortunately, breaks "classic" tasks that need this:
export default class MyComponent extends Component.extend({
myClassicTask: task(function *(this: MyComponent) {
if (this.foo) {
⋮
})
}) {
foo = true;
⋮
}Results in: tsserver: (2506) 'MyComponent' is referenced directly or indirectly in its own base expression.
The typedefs currently in ember-osf-web sacrifice type safety to make this work (which I'm not happy with, of course).
There is a workaround, which is to use an interface:
interface IMyComponent {
foo: boolean;
}
export default class MyComponent extends Component.extend({
myClassicTask: task(function *(this: IMyComponent) {
if (this.foo) {
⋮
})
}) {
foo = true;
⋮
}which works, but is not super great. Of course the real solution is to move these tasks into the class body and use ember-concurrency-decorators, but for us (at least) we were hoping to be able to do that incrementally. I still think what you have here is correct, we just might need to communicate the caveat that it will break "classic" tasks and suggest the workaround (or another better one, if it exists).
There was a problem hiding this comment.
^ this is some great feedback, and another good reason to let tests (both for classic and octane use cases) be your guide
There was a problem hiding this comment.
we just might need to communicate the caveat that it will break "classic" tasks and suggest the workaround (or another better one, if it exists).
I'm not sure I agree with this, given that vscode uses type information now even for apps that do not have type-checking enabled. One of the immediate wins of landing this would be having in-editor documentation/autocomplete around the Task, TaskInstance, TaskGroup APIs. If anything, "classic" usage may be significantly more important to cater to than octane usage at this time.
I still feel that we should cover both as part of any initial release of type information to ember-concurrency. See details in the previous PR here: #209 (comment)
There was a problem hiding this comment.
I agree, but also don't know how we'll make this work. As soon as you type taskFn as a callable (even () => any) you get the error about referencing the class in its own base expression, because, well, you are. When it's typed as just any (what we've been doing in our app) I'm guessing the compiler just ignores it outside the function. There may be a way to type this so that it will work, but I'm not seeing it.
| /** | ||
| * Retrieves the value of a property from the object. | ||
| */ | ||
| get<K extends keyof this>(key: K): UnwrapComputedPropertyGetter<this[K]>; |
There was a problem hiding this comment.
We have the same in DT: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/de12907d79777d50ce08036827da7425853abefd/types/ember__object/observable.d.ts#L12
Don't we just say getting in this way is not supported when using TS?
| import { | ||
| UnwrapComputedPropertyGetter, | ||
| UnwrapComputedPropertyGetters | ||
| } from '@ember/object/-private/types'; |
| * `TaskInstances`, and at any point, you can call the `.cancelAll()` method | ||
| * on this object to cancel all running or enqueued `TaskInstance`s. | ||
| */ | ||
| export interface Task<Args extends any[], T> extends Getter { |
There was a problem hiding this comment.
Shouldn't this extend TaskProperty? Otherwise, we don't get the modifiers.
| | InstanceRunning | ||
| | InstanceWaiting | ||
| | InstanceError | ||
| | InstanceSuccess); |
There was a problem hiding this comment.
This is cool because it guarantees valid state properties, but it also seems to break .get(). For:
const taskInstance = this.myTask.perform();
taskInstance.get('isRunning');I get:
This expression is not callable.
Each member of the union type '(<K extends "error" | "get" | "getProperties" | "hasStarted" | "isCanceled" | "isDropped" | "isFinished" | "isRunning" | "isSuccessful" | "value" | "cancel" | "catch" | "finally" | "then" | "state">(key: K) => UnwrapComputedPropertyGetter<...>) | ... 4 more ... | (<K extends "error" | ... 13 more ... | "state">(key:...' has signatures, but none of those signatures are compatible with each other.
There was a problem hiding this comment.
It’s worth seeing if this works if we drop the magic bits imported for get—I haven’t had a chance to test, but I think it may, between that and extending from EmberObject? Because at that point it should be actually the same underlying type definition being invoked.
There was a problem hiding this comment.
Extending from EmberObject results in the same error. Removing the union of state interfaces works and also intersection with just one of the state interfaces works. Intersecting with the union of two or more of the states breaks. It looks like UnwrapComputedPropertyGetter can't handle unions of interfaces with different signatures, which makes sense given that it uses infer.
| * Describes the state that the task instance is in. | ||
| * Can be used for debugging, or potentially driving some UI state. | ||
| */ | ||
| export enum TaskInstanceState { |
There was a problem hiding this comment.
|
|
||
| export function taskGroup(): TaskGroupProperty; | ||
|
|
||
| interface CommonTaskProperty { |
There was a problem hiding this comment.
should this also have .withTestWaiter? or is it recommended that people use the wrapping version?
|
I think this method should go somewhere: 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>;
}
This is more in conjunction with ember-concurrency-decorators though
This only required that I change the perform(...args: PerformArgs): TaskInstance<PerformReturn>;(and then passed the supporting generics around) |
|
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 |
This copies the types from machty/ember-concurrency-decorators#56 and together with that PR will finally enable type-safe usage of ember-concurrency.
Closes #209, though we should compare both.
Not 💯 sure what's left to do here and whether / how to test this.
/cc @jamescdavis @chriskrycho @dfreeman @josemarluedke