machty / ember-concurrency

ember-concurrency is an Ember Addon that enables you to write concise, worry-free, cancelable, restartable, asynchronous tasks.
http://ember-concurrency.com
MIT License
691 stars 155 forks source link

Use the types published from Ember itself #512

Closed gitKrystan closed 1 year ago

gitKrystan commented 1 year ago

One way to resolve https://github.com/machty/ember-concurrency/issues/510

This breaks the "unwrapping" of Tasks when using get.

Since we are no longer getting the benefits of using ComputedProperty for unwrapping of get, tomorrow I will add another commit removing that hack.

gitKrystan commented 1 year ago

Relevant: https://github.com/emberjs/ember.js/pull/20387#issuecomment-1450074350

ef4 commented 1 year ago

I'm using this patch in an app and it's working well.

gitKrystan commented 1 year ago

@dfreeman made a comment on a related PR (https://github.com/emberjs/ember.js/pull/20387#issuecomment-1450074350) that I looked into today.

...the type of this.foo and get(this, 'foo') should be the same once you take ComputedProperty out of the equation, so if this.someTask.perform(...) works, then get(this, 'someTask').perform(...) should also work. That's what this signature should be accomplishing.

Most of the type tests in this library were written pre-Octane, so they use EmberObject.extend({}) and task(function* () {}) which returns the type TaskProperty. In this case, while this.foo and this.get('foo') do have the same type as indicated in Dan's comment, the type is TaskProperty rather than Task. This is because the task function returns a "computed-like" TaskProperty when passed a generator. In some cases, this can be handled with the taskFor function from ember-concurrency-ts. In other cases, I needed to use casts.

In the newer Octane tests that use the async arrow function syntax, this.foo has the correct type. get(this, 'foo') is still not working properly, and I don't understand why. The returned type seems to be this['foo'] which TS seems to be translating to any??? (Or at least, the not.toBeAny() is failing with a cryptic message.) See here (The test for "async arrow with get").

dfreeman commented 1 year ago

the type is TaskProperty rather than Task

I think the key piece that I was trying to communicate in that comment that may have gotten lost in the noise is this:

instead of TaskProperty extends ComputedProperty<Task<...>>, I think ember-concurrency should be able to directly do TaskProperty extends Task<...>

In other words, anywhere in any old code that's been using ComputedProperty<T>, we essentially now want to just use T, for all the reasons we talked about over in that PR about get no longer doing anything special. Do note that all of this discussion only applies to pre-Octane Object.extend(...) stuff (or at least conceptually it does; depending on how the specific types are entangled with one another in ember-concurrency there could be some cross-edition impact)

The returned type seems to be this['foo'] which TS seems to be translating to any??? (Or at least, the not.toBeAny() is failing with a cryptic message.)

I think this is the intersection of two facts:

If actually invoking perform on the result of the get call works, then I think you're good đź‘Ť

ms: number | undefined = 500

This is suuuuper weird that it's inferring ms: any without the annotation. I'm guessing it's to do with how inference is working with all the AsyncTaskArrowFunction stuff, but I have no clue how/why that would have changed when swapping out the DT types for the native ones, since it doesn't seem like any of this inference actually depends on the Ember types.

gitKrystan commented 1 year ago

@dfreeman I pushed some changes addressing your comment.

I believe the issue with defaulted arguments becoming any predates these changes because I was able to reproduce it on my project that uses the latest e-c release.

I've been investigating how to fix it, but no luck so far.

edit: Indeed this is an existing issue:

https://github.com/machty/ember-concurrency/blob/b0acd665d08c0ba795c3f1cc13b2b7cf12b0b048/tests/types/ember-concurrency-test.ts#L2880-L2881