Closed jamescdavis closed 4 years ago
FWIW, I very much prefer this approach! It avoids introducing any other new API surface, and looks like it should be easy to iterate toward a “resources”-based approach (the @use
proposal). cc. @chancancode @maxfierke @dfreeman @pzuraq
I'm also a big fan of this, particularly if ember-concurrency-async
can be updated to work with it!
Those two things together would deliver end-to-end type safety without putting the burden on the caller to remember to use taskFor
anywhere they want to interact with a task ✨
https://github.com/chancancode/ember-concurrency-async/pull/6 makes this work with async tasks e.g.
class MyClass {
one = 1;
@task
myTask = task(async function(this: MyClass) {
await this.one;
return 2;
});
@task
myTask = task(async () => {
await this.one;
return 2;
});
}
AsyncTask types and tests for async tasks w/ task wrapper are here: https://github.com/jamescdavis/ember-concurrency-decorators/pull/1 but we obviously can't drop them in like that. Need to think about how to make them manually importable like ember-concerrency-async
s are.
It would type correctly but behave in an undesirable ay: assignment to a class field is a per-instance setup, unlike methods which end up on the prototype. Decorators can transform properties or methods, and can in fact transform the property into a prototype-bound method. Using the two in combination is definitely odd, but it’s what’s necessary for now.
It’s also close to what we’ll likely end up with in a resources-driven design in the future, where a task usage might look something like this:
import Component from '@glimmer/component';
import { use } from '@ember/something-something-resources';
import { task } from 'ember-concurrency';
export default class MyComponent extends Component {
@use myTask = task(function*() {
// ... the normal task body ...
});
}
What @chriskrycho said and it throws an error: https://ember-twiddle.com/55d9db44e9958d7fab70fe1fa91e218b?openFiles=components.undecorated-task%5C.js%2C
The double usage of task
is a little different, I agree, but the alternative would be to introduce a new thing to import, which seems not great and is further from the @use
pattern @chriskrycho mentioned. Also, not sure what we would call it.
Some more thoughts about this design that I had a when I proposed it @buschtoens:
export default Component.extend({
myTask: task(function*() {});
});
|| V
export default class MyComponent extends Component {
@task
myTask = task(function*() {});
}
making it easier for people migrating to ES6 classes/Octane.
A number of the decorators now built-in to Ember core serve "double duty" and can be used as a decorator and a regular function, e.g.
import { computed } from '@ember/object';
export default class extends Component {
@computed('something')
get doubleSomething() {
return this.something * 2;
}
}
and
import { computed } from '@ember/object';
export default Component.extend({
doubleSomething: computed('something', function() {
return this.something * 2;
}
});
This is obviously not the same thing because, in the latter, computed
actually does something rather than pass through and in what I'm proposing here, we'd use task
as a decorator and a function at the same time, but I just wanted to point out that the concept of an import switching behavior based on usage is not new in Ember.
I don't think it's the overloading of task
that I have concerns about so much as it's task
from ember-concurrency-decorators
being overloaded and not task
from ember-concurrency
. It's becoming clearer from the past few months of more people adopting ES classes and using decorators with ember-concurrency, that it's probably inevitable that we merge the decorator approach here into upstream ember-concurrency
at some point. So my concern is that this approach might lock us out from doing that or make that usage more confusing, if it is possible to retain these semantics in some clever merging. Is the function wrapping here at all in conflict with task
from ember-concurrency
?
I do find the @use
use-case (lol) compelling for this though!
So, you're thinking of making import { task } from 'ember-concurrency';
also work as a decorator? Yeah, that would conflict because we'd have no way to tell whether a functional invocation of task
should just pass through the generator function or create a task with it. In that case, we'd have to call this pass-through-wrapper-for-types thing something else. Any ideas for a name?
If only decorators could affect type...
getTask()
?
makeTask()
?
typeAsTask()
?
makeThisThingATaskAsFarAsTypeScriptIsConcerned()
?
trustMeItsATask()
?
I think my preferences would lie with something like castToTask
, toTask
, or asTask
? Obviously, these are all a bit longer to type than task
, but perhaps someone will figure out a nice import alias.
Now that you mention it, I believe asTask
was my original suggestion before we landed on just overloading task
. That fits well with TypeScript's as
keyword for asserting type, which is pretty close to what we're doing here. It's basically the same as:
@task
myTask = (function*(something: string) {
yield something;
return 42;
} as unknown) as ComputedProperty<Task<number, [string]> & EmberObject>;
except that it automatically infers the return vale and arguments and isn't ugly as hell.
What do we think about?:
@task
myTask = asTask(function*(something: string) {
yield something;
return 42;
});
React with: :+1: , :-1:, or 😕
As far as I can tell, if we are not overloading, then that is pretty much the same signature as taskFor
, and if you prefer to put taskFor
around the assignment instead of on the usage side, it will Just Work™? Not opposed to exploring this or merging the addons (it certainly doesn't make it easy for me to maintain), but checking if I am missing something.
@chancancode well, actually, yeah! taskFor
does exactly the same thing if we're not overloading and could definitely be used at assignment (which is very much my preference and I believe that of some others but I don't want to speak for them). It would be nice if it were part of ember-concurrency-decorators
so there wasn't another package to install and IMHO the name is not as clear as to what it does as asTask
, but otherwise this works fine:
import { taskFor } from 'ember-concurrency-ts';
export default class MyComponent extends Component {
@task
myTask = taskFor(function*(something: string) {
yield something;
return 42;
});
}
as well as
import { taskFor } from 'ember-concurrency-ts';
export default class MyComponent extends Component {
theAnswer = 42;
@task
myTask = taskFor(async (something: string) => {
await something;
return this.theAnswer;
});
}
when coupled with a slightly adjusted https://github.com/chancancode/ember-concurrency-async/pull/6.
So maybe there's actually nothing new here. ¯\_(ツ)_/¯
I would like to see this usage illustrated as an option on http://ember-concurrency.com/docs/typescript (and in ember-concurrency-ts
).
Well, works fine for types. We'd have to adjust this assertion for taskFor
to work for assignment: https://github.com/chancancode/ember-concurrency-ts/blob/05795edb1ae8cd5f6818d36aaf2ca9cb74d4f5f4/addon/index.js#L6
Here's a PR that does that: https://github.com/chancancode/ember-concurrency-ts/pull/1
Here's a PR that makes using taskFor
at assignment work with async: https://github.com/chancancode/ember-concurrency-async/pull/7 (tests won't pass without https://github.com/chancancode/ember-concurrency-ts/pull/1 but the transform works).
Closing in favor of https://github.com/chancancode/ember-concurrency-ts/pull/1
This is based on #56 now that types have landed in
ember-concurrency
.The idea is that
task
can act as both a decorator and as a function that wraps a task generator or encapsulated task descriptor. When used as a wrapper, it simply passes through the argument while providing a return type ofTask
. This allows you to interact with the task property exactly as you would in JavaScript, but with the type-safety of TypeScript:Note: This is an alternative approach to https://github.com/chancancode/ember-concurrency-ts
@chancancode @dfreeman @chriskrycho @buschtoens