Add typedefinitions #209
Add typedefinitions #209
Conversation
Signed-off-by: Arnav Gupta <[email protected]>
|
Possibly useful for comparison: here are mine as a Gist. |
|
I have no TypeScript experience to understand what all of this means/does or how to compare @championswimmer's and @chriskrycho's solutions; could you (and other one else) explain/compare/contrast? |
|
I'll take a look later this week and compare. Cc. also @dwickern and @mike-north, both of whom have looked at this before. |
|
@chriskrycho |
|
The typings @chriskrycho linked also supports typed arguments to |
types/Task.d.ts
Outdated
| enqueue(): void; | ||
| group(groupPath: any): any; | ||
| keepLatest(): void; | ||
| maxConcurrency(n: number): void; |
There was a problem hiding this comment.
Many (all?) of these should be chainable options, and thus cannot return void
export default Controller.extend({
restartableTask3: task(SHARED_TASK_FN).maxConcurrency(3).restartable(),
enqueuedTask3: task(SHARED_TASK_FN).maxConcurrency(3).enqueue(),
droppingTask3: task(SHARED_TASK_FN).maxConcurrency(3).drop(),
keepLatestTask3: task(SHARED_TASK_FN).maxConcurrency(3).keepLatest(),
});There was a problem hiding this comment.
Yeah correct, just saw that
types/Task.d.ts
Outdated
| readonly isSuccessful: boolean; | ||
|
|
||
| /** Describes the state that the task instance is in. Can be used for debugging, or potentially driving some UI state. */ | ||
| readonly state: 'dropped' | 'cancelled' | 'finished' | 'running' | 'waiting'; |
There was a problem hiding this comment.
Might be worth breaking this value type out into its own alias, for reuse in libraries/apps that consume ember-concurrency.
types/Task.d.ts
Outdated
|
|
||
| finally(): RSVP.Promise<any>; | ||
|
|
||
| then(): RSVP.Promise<any>; |
There was a problem hiding this comment.
Looks like a TaskInstance should be usable as a promise-like thing. Shouldn't then/catch/finally take function arguments, and be generic over the types those callbacks resolve to?
There was a problem hiding this comment.
There is a type for Promise.ThenFunction and Promise.CatchFunction. will use those
|
@machty and co. - at risk of stating the obvious, misaligned type information runs the risk of providing a poor dev experience (even worse than the complete absence of type information). At present, anything involving generator functions is a bit tricky due to gaps in TS (this is the main thing we're waiting on -- as I understand it the underlying type system can handle this already, it's just a debate over the right syntax) Even after iterating on this, we may want to cut a pre-release and get a few people consuming this before considering a stable release. |
Signed-off-by: Arnav Gupta <[email protected]>
Signed-off-by: Arnav Gupta <[email protected]>
|
@mike-north @chriskrycho @machty |
|
I'll take a look later; and in particular I will drop it in in our app and make sure it covers all the cases my own typings linked above cover, and if not, suggest how to make sure it does! Thanks for working hard on this! |
|
Any updates 🙈 |
|
Yes, sorry; ended up very swamped this week. I've taken some time with this and will block out an hour to dig in on Monday. I can say from reading it carefully that we do need to pull in some of the things my typings have, because they give you completion and type checking for |
|
@championswimmer I'm super sorry this keeps getting delayed. We're trying to ship some stuff at work and have hit roadblock after roadblock basically since I got back from EmberConf. 😭 I have a handful of things I'm blocking out time for today, and this is one of them. I'll probably open a PR against your PR to hit the couple pieces I see are missing. You've got a fair number of things I missed, and vice versa! |
|
And we ran into more derailing today. I will get to this; I apologize for the many delays. |
|
Haha, no problem @chriskrycho :) |
|
Ping @chriskrycho, we will all be incredibly grateful once this lands. 😇 |
|
(i have no idea what I'm talking about) is this something that we can just merge and tweak/improve over time? or is there a lot of damage if we publish something not 100% perfect? |
|
Sorry; this has been a ridiculous month.
We shouldn’t merge these as is; but it’s genuinely only an hour or so of work to get them mergeable. I have four hours blocked out next week for TS stuff at work, and this is near the top of that list.
|
|
@chriskrycho got any time this week 🙈 |
|
😭 maybe. Hopefully, even!
|
|
Any news on this? I would love to get this as well. |
|
As my friends back in North Carloina might have said: “Lord wiling and the creek don’t rise…” I’ll get to this on Tuesday. |
|
@chriskrycho |
|
UP |
TLDRI feel pretty strongly that a decorator(s) should be considered as part of an MVP for the first stable ember-concurrency release that contains type information. I've just spent about a week focusing on Ember+TS ergonomics, and type definitions for ember-concurrency have become one of the "sore thumbs" that stick out (only because so much excellent work has been put into other areas). Currently (with types much like those in this PR) the way you use e-c with TypeScript is like is like // This pattern is equivalent to typical use of ember-concurrency
export default class XFoo extends Component.extend({
// stuff that MUST go on the prototype goes up here
doSearch: task(function*(this: XFoo) {
const title = this.get('title');
...
})
}) {
public title = '';
public description = '';
// vast majority of other interesting stuff goes down here.
}One challenge (trap?) teams are facing is that there's a "cleaner looking" pattern that's 100% wrong, and the lions share of those new to ES6 classes and TypeScript seem to want to use it //// THE PATTERN BELOW IS WRONG AND SHOULD NOT BE USED ////
// TS (or ES6 w/ class fields) version
export default class XFoo extends Component {
public title = '';
public description = '';
public doSearch = task(function*(this: XFoo) {
const title = this.get('title');
...
});
}
// ES6 version
export default class XFoo extends Component {
constructor() {
super();
this.doSearch = task(function*(this: XFoo) {
const title = this.get('title');
...
});
}
}At its root, there are several factors that allow teams to fall into this trap, including
I feel pretty strongly that a decorator(s) should be considered as part of an MVP for the first stable ember-concurrency release that contains type information. Consumers don't have to use it, they should be there, and the recommended method of consumption. The goal of this decision would be to guide developers toward ensuring their e-c tasks are defined on prototypes and not each instance. Decorator(s) could live in a separate addon (i.e., live in ember-decorators), but the release(s) should be coordinated such that ES6/TS consumers are encouraged to use them (or at least nearly 100% of consumers in this group should know the approach is available and less error-prone). Example usage might look like import Component from '@ember/component'
import { task, restartable, maxConcurrency } from 'ember-decorators/task';
export default class XFoo extends Component {
public title = '';
public description = '';
@maxConcurrency(3)
@restartable
@task
public doSearch*() {
const title = this.get('title');
...
}
} |
|
@mike-north Though to be fair to the community, there are many instances, like VueJS where the creators have actively put in hours of effort (both in the JS implementation and in creating the required typedefs) to make |
|
I have been exploring approaches in this space (not just sitting on it, though I've been quiet) for the last several weeks, and I broadly concur with @mike-north's take here. In particular, this –
– is the kind of thing that is easy to get wrong even if you do grok the model. I missed it, and just in the last week I had an Ember Core person note that they had missed it as well when I first recommended doing property assignment. Chris Garrett and I have both been actively documenting this as I can, and I've got a blog post coming Monday or Tuesday which I'll publicize as widely as possible to support it. The other thing we need to sort out here is how we want to approach typing these. My own typings are more robust than the typings in this pull request, but my experience is that TS's inference ends up falling over with them around half the time – it just doesn't do well with closure arguments at this point. I'll have more to say on this next week, but given the many limitations in TS with generator typings at present, I'm currently inclined toward a more conservative correct-but-not-complete approach, which is I think the best we can actually do here until TS lands proper support for generators. |
|
I dug into this even more, and unfortunately have bad news x2 1. My "right way to do e-c w/ TypeScript" is even worse than stated before.export default class XFoo extends Component.extend({
// stuff that MUST go on the prototype goes up here
doSearch: task(function*(this: XFoo) { // << PROBLEM
const title = this.get('title');
...
})
}) {
public title = '';
public description = '';
// vast majority of other interesting stuff goes down here.
}The example above is not valid because we're forbidden from referring to the This, however, does work -- the task ends up on the prototype and with the correct type. export default class XFoo extends Component {
public doSearch!: Task;
public title = '';
public description = '';
}
XFoo.prototype.doSearch = task(function*(this: XFoo) { // << PROBLEM
const title = this.get('title');
...
});Maybe @chriskrycho's more robust e-c types solve the generator function lexical scope issue, and will save us from having to do this kind of fugly stuff? 🙏🙏🙏🙏🙏 2. TS method/attribute decorator types do not support altering typessee: microsoft/TypeScript#4881 - a discussion about class decorators that covers similar concerns Essentially, if I have something like this export default class Foo extends Controller {
// normal class body definition here
@task
public *countdown() {
yield timeout(1000);
}
@action
public startCountdown() {
this.get('countdown').perform(); // 💥
}
}There's no supported way to tell TypeScript that we've altered the shape of the JS decorators in their current (WIP) state are flexible enough that they can do what needs to be done to create the nice syntax above, but there's a fundamental limitation that would stop us from accurately describing this decorator in ambient type information. |
|
Unfortunately, the scope issue is what led me to experiment with class fields in the first place. 😞 There’s no workaround for it in the traditional Ember Object world that gets you the relevant types without running into exactly what you found here. The only other thing I’ve thought of (but haven’t tried) is defining the generator function outside the class, including its this argument, and binding it in the prototype. I suspect that will just show the same type error, though. |
|
Let's warm up this topic. Today, we have decorators for tasks 🎉 Also I did created my own typings, too. Because I find existing ones just afterwards, they are here: https://github.com/gossi/spomoda/blob/master/client/types/ember-concurrency/index.d.ts What makes me curious is the way to import a task.
As this might be another aspect (since the code still looks totally different) and might have an impact on it (when it may be will refactored ... to TS?). |
|
Awaiting this PR with excitement. As it is so far, I don't think any the proposed solutions cover encapsulated tasks. For example foo = task({
count: 1,
*perform() {
this.incrementProperty('count');
// do other stuff
}
}) |
|
I just had an idea and would love your opinions: machty/ember-concurrency-decorators#30 |
|
I opened #319 to have another go at this. |
|
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 |
Signed-off-by: Arnav Gupta [email protected]