-
-
Notifications
You must be signed in to change notification settings - Fork 157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add waitFor
support to async arrow function API
#531
base: master
Are you sure you want to change the base?
Conversation
`waitFor` from `@ember/test-waiters` can be applied to async functions and generator functions. But there's not currently any way to apply them to async arrow task functions in a way that will work with the babel transpilation. So this commit adds a `waitFor` task option only when using async arrow functions that allows the caller to pass in the `waitFor` function, and causes it to be applied to the transpiled-output generator function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I like the approach. I think it's probably a reasonable expansion of the API as there's potentially other implementations someone might want to provide in advanced cases, so I appreciate it not being strictly coupled to the @ember/test-waiters
implementation
I do think it should be implemented as modifier like the one suggested in #513, with the exception of its option
being the waitFor
function to use instead of a boolean.
One could make the argument that the modifier could be implemented and live in @ember/test-waiters
instead, but that would require extending types or something and I'm not exactly sure how that works in practice.
@@ -3128,6 +3129,7 @@ module('integration tests', () => { | |||
debug = task(this, { debug: true }, async () => {}); | |||
onState = task(this, { onState: () => {} }, async () => {}); | |||
onStateNull = task(this, { onState: null }, async () => {}); | |||
waitFor = task({ waitFor }, async () => {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is missing the this
(otherwise, it's the same test case as the one further down):
waitFor = task({ waitFor }, async () => {}); | |
waitFor = task(this, { waitFor }, async () => {}); |
if (optionsWithBufferPolicy && optionsWithBufferPolicy.waitFor) { | ||
result.generator = optionsWithBufferPolicy.waitFor(result.generator); | ||
|
||
optionsWithBufferPolicy = Object.assign({}, optionsWithBufferPolicy); | ||
delete optionsWithBufferPolicy.waitFor; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this probably should be moved into a modifier (a la #513), so that it works regardless of whether a task is running through the AyncArrowRuntime or not
type AsyncArrowFunctionTaskOptions< | ||
HostObject, | ||
T, | ||
Args extends any[] | ||
> = TaskOptions & { | ||
waitFor?: ( | ||
fn: AsyncArrowTaskFunction<HostObject, T, Args> | ||
) => AsyncArrowTaskFunction<HostObject, T, Args>; | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might not be needed to be special-cased if moved to a waitFor
modifier
It's not allowing a task to be wrapped in something is good though. May not even have to be a |
I am opening this PR to start a discussion. There is currently no way I can find to wrap async arrow function tasks in a test waiter a la
waitFor
from@ember/test-waiters
. So this is a stab at adding support for that -- see the commit message for an explanation of this solution.I haven't updated any documentation or anything because I want to see if this is a direction we want to pursue first.
A couple of alternatives I thought of:
myTask = task(waitFor(async () => { /* ... */ }));
would produce{ generator: waitFor(function*() { /* ... */ }) }
. This seems sketchy because in the source code the modifier function would be called with anasync
function while in the transpiled code it would be called with a generator function. This happens to work forwaitFor
, but providing generic support for modifier functions like this would come with a pretty screwy caveat that they must support being passed generator functions even though they must be typed to acceptasync
functions.waitFor
and then do something likemyTask = task({ modifiers: [ waitFor ] }, async () => { /* ... */ });
. This would allow us to type the modifier functions to accept a generator function without running into trouble because the un-transpiled source code would never invoke the modifier function. On the other hand, it complicates the syntax and it's not clear there will ever be a need for other modifier functions like this. I'm happy to explore this route if we prefer -- I don't think it would be much more work.waitForPromise()
call, or work with the@ember/test-waiters
maintainers to try to devise some other easy way to wrapember-concurrency
tasks.I'm very interested to hear folks' thoughts as my app relies heavily on
@waitFor
, and I really love the async arrow function syntax and want to move to it soon as I can figure out what to do about the@waitFor
s!Related to #513