Support waitFor()-type modifiers in async arrow transform

[bendemboski]

Update the async arrow task babel transform to allow the task function to be wrapped in any "modifier functions" such as waitFor from @ember/test-waiters, and preserve that wrapping in the output.

Note that this is a little screwy from a types perspective because these modifier functions must by typed to accept an async function, but at runtime are actually passed generator functions. This is fine for waitFor() because it accepts both, and maybe this is the only modifier function we'll ever need to support, but it is a kinda funny caveat.

[bendemboski]

This is an alternative to #531

[maxfierke]

Overall, approach looks good to me. Appreciated all the comments, as well. Left a couple of small suggestions. Looks like there's some interaction with @embroider/macros based on the CI results, though.

[bendemboski]

@maxfierke I addressed your two pieces of feedback along with a couple more clarifying comments (and rebased them into the original commit). Once #537 is merged I'll rebase this onto master and we should have a "passing" build (aside from the embroider scenarios).

If we're feeling confident we're going to go with this approach, then I could use some advice on updating documentation -- I'm not really sure where it makes sense to mention this support for modifier functions. Also, I'm not sure if it's worth talking about it in general, or just document the waitFor() usage since that's the main/only use case I know of for it right now? Thoughts?

[maxfierke]

If we're feeling confident we're going to go with this approach, then I could use some advice on updating documentation -- I'm not really sure where it makes sense to mention this support for modifier functions. Also, I'm not sure if it's worth talking about it in general, or just document the waitFor() usage since that's the main/only use case I know of for it right now? Thoughts?

I think it's probably reasonable to stick to documenting the waitFor usage, since that's really all that it's being used/recommended for. It's useful to have this capability in the transform, but I think in general, wrapping for other uses is probably best left to a modifier, given the typing caveats with generalizing this approach.

If you're looking for a spot, the "Testing/Debugging" page is probably a good place since it'll be @ember/test-waiters-specific.

[bendemboski]

@maxfierke I rebased so the build is passing.

I'm actually thinking maybe we don't need to add documentation here? We didn't have explicit document for the waitFor() usage before, and just rely on the documentation in @ember/test-waiters. I can open a PR over there to update the ember-conciurrency usage examples to include the new/arrow function syntax, but unless you see the need for documentation here, I think this is ready to merge?

[maxfierke]

looks good to me! I'll get this merged & released this weekend

I'm actually thinking maybe we don't need to add documentation here? We didn't have explicit document for the waitFor() usage before, and just rely on the documentation in @ember/test-waiters

I do think it'd be useful to at least link out to those @ember/test-waiters docs from the e-c docs once they're available, but we can do that in a follow-up PR

[Techn1x]

Glad to see work on this! I've been using ember-test-waiters with ember-concurrency for a while and had a lot of trouble here.

Is this the expected syntax for usage?

queryStats = task(
  { restartable: true },
  waitFor(async (query: typeof this.query) => {
    // stuff
  }),
)

or this with the decorator?

@waitFor
queryStats = task({ restartable: true }, async (query: typeof this.query) => {
  // stuff
}),
)

Also, types seem to be an issue :/ is this something for e-c to fix or for ember-test-waiters? There's an open issue over here
emberjs/ember-test-waiters#241

No overload matches this call.
  Overload 1 of 9, '(target: Object, propertyKey: string): void', gave the following error.
    Argument of type 'Function' is not assignable to parameter of type 'string'.
  Overload 2 of 9, '(hostObject: { restartable: boolean; }, asyncArrowTaskFn: AsyncArrowTaskFunction<{ restartable: boolean; }, any, any[]>): TaskForAsyncTaskFunction<{ restartable: boolean; }, AsyncArrowTaskFunction<{ ...; }, any, any[]>>', gave the following error.
    Argument of type 'Function' is not assignable to parameter of type 'AsyncArrowTaskFunction<{ restartable: boolean; }, any, any[]>'.
      Type 'Function' provides no match for the signature '(this: { restartable: boolean; }, ...args: any[]): Promise<any>'.
  Overload 3 of 9, '(baseOptions: { restartable: true; }, asyncArrowTaskFn: AsyncArrowTaskFunction<unknown, any, any[]>): TaskForAsyncTaskFunction<unknown, AsyncArrowTaskFunction<unknown, any, any[]>>', gave the following error.
    Argument of type 'Function' is not assignable to parameter of type 'AsyncArrowTaskFunction<unknown, any, any[]>'

[bendemboski]

@Techn1x the first non-decorator usage is the correct one. It's not typechecking because the waitFor types in @ember/test-waiters are problematic until this PR is merged and released.

In the meantime I'd probably recommend using patch-package to patch that change into @ember/test-waiters (or do it "natively" if you're using pnpm).

[Techn1x]

Ah! Fantastic. Didn't see that PR, I'll go link it to the issue over there and hopefully that'll help. Thanks a bunch!

I am using pnpm so I'll probably patch it with that in the meantime

[Techn1x]

So I switched to the non-decorator usage but noticed the types are a little different now.

Would this type issue end up being fixed by that PR as well?

[bendemboski]

@Techn1x I'm not sure...does it still produce the error if you remove the waitFor and just have a (non-decorator) task with a "bare" arrow function?

[chrisvdp]

@Techn1x @bendemboski I tried the upstream change locally and can see that it resolved those issues.