Ava: Remove observable support, generators, only type native promises?

[novemberborn]

AVA currently supports observables, returned by test implementations and used in t.throws() / t.notThrows() assertions. We're not happy with how they're typed though, see #1685. Their progress through TC39 has also been slower than expected.

I wonder whether we should remove support in AVA itself? Though less graceful, it wouldn't be hard to wrap observable-returning functions and do the same translation to a promise that AVA does at the moment. As precedent there are other asynchronous concepts in Node.js that we don't support directly (streams, async iterators).

This would simplify our typings for t.throws(), which hurt usability in Flowtype (#1790).

I'm not convinced we need to support generator functions as test implementations either.

Similarly, we support tests that return thenables, rather than promises. Typing these "promise-like" objects is also more difficult than expected (#1778). At the very least we could change the typings to expect a Promise. I don't quite think it's worthwhile enforcing tests to return proper Promise instances rather than thenables, and properly typed promise implementations should match the Promise interface anyhow.

---

If you're using observables or generators, how would you feel if we'd remove first-class support?

Similarly, would it be a terrible bother if our typings were stricter on promises?

@avajs/core, any thoughts?

[hallettj]

tl;dr: I think that the simplest option is to split throws into multiple assertions: throws for functions that should throw a synchronous error, rejects for functions that should return a rejected promise, and emitsError for functions that return an observable that should emit an error. The assertions for a rejected promise and an error-emitting observable could reasonably be combined into one. But I don't see a good way to overload throws to handle both synchronous and asynchronous behavior. Another option is to change throws so that it always returns the error in a Promise.

The root of the issue is overloading a function signature in a way that requires Flow to look ahead to determine which signature to select. In theory Flow could figure this out if it considered all possible signatures, and backtracked as necessary. In practice the Flow team have found that too much backtracking leads to unacceptably slow type checking. So they limit the number of steps Flow will look ahead before it must identify the correct "path" to choose. At that point if Flow cannot unambiguously pick a signature then it reports an error.

The type for the signatures of t.throws looks like this (with most comments removed):

export interface ThrowsAssertion {
    (fn: () => ObservableLike, expectations?: null, message?: string): Promise<any>;
    (fn: () => ObservableLike, constructor: Constructor, message?: string): Promise<any>;
    (fn: () => ObservableLike, regex: RegExp, message?: string): Promise<any>;
    (fn: () => ObservableLike, errorMessage: string, message?: string): Promise<any>;
    (fn: () => ObservableLike, expectations: ThrowsExpectation, message?: string): Promise<any>;
    (fn: () => PromiseLike<any>, expectations?: null, message?: string): Promise<any>;
    (fn: () => PromiseLike<any>, constructor: Constructor, message?: string): Promise<any>;
    (fn: () => PromiseLike<any>, regex: RegExp, message?: string): Promise<any>;
    (fn: () => PromiseLike<any>, errorMessage: string, message?: string): Promise<any>;
    (fn: () => PromiseLike<any>, expectations: ThrowsExpectation, message?: string): Promise<any>;
    (fn: () => any, expectations?: null, message?: string): any;
    (fn: () => any, constructor: Constructor, message?: string): any;
    (fn: () => any, regex: RegExp, message?: string): any;
    (fn: () => any, errorMessage: string, message?: string): any;
    (fn: () => any, expectations: ThrowsExpectation, message?: string): any;
    (promise: ObservableLike, expectations?: null, message?: string): Promise<any>;
    (promise: ObservableLike, constructor: Constructor, message?: string): Promise<any>;
    (promise: ObservableLike, regex: RegExp, message?: string): Promise<any>;
    (promise: ObservableLike, errorMessage: string, message?: string): Promise<any>;
    (promise: ObservableLike, expectations: ThrowsExpectation, message?: string): Promise<any>;
    (promise: PromiseLike<any>, expectations?: null, message?: string): Promise<any>;
    (promise: PromiseLike<any>, constructor: Constructor, message?: string): Promise<any>;
    (promise: PromiseLike<any>, regex: RegExp, message?: string): Promise<any>;
    (promise: PromiseLike<any>, errorMessage: string, message?: string): Promise<any>;
    (promise: PromiseLike<any>, expectations: ThrowsExpectation, message?: string): Promise<any>;

    /** Skip this assertion. */
    skip(thrower: any, expectations?: any, message?: string): void;
}

There are only two possible return types; so a lot of these signatures can be combined without losing
precision, which does make things somewhat easier for Flow. We just need to express that if the first argument is a function that returns a PromiseLike or ObservableLike then the return type is a Promise; otherwise the return type is not a Promise (which is expressed here as any but perhaps could be given as ?Error); and there are some requirements on what may be given for the optional second and third arguments:

type ErrorExpectations = Constructor | RegExp | ThrowsExpectation | string

export interface ThrowsAssertion {
    (
        fn: (() => PromiseLike<any>) | (() => ObservableLike),
        expectations?: null | ErrorExpectations,
        message?: string,
    ): Promise<any>;
    (
        fn: () => any,
        expectations?: null | ErrorExpectations,
        message?: string,
    ): any;

    /** Skip this assertion. */
    skip(thrower: any, expectations?: any, message?: string): void;
}

But that still does not solve the problem of #1790! There are still two signatures to choose between, and the second signature will always be valid if the first signature is valid. If we change throws to behave only according to the second signature, and create a new assertion called rejects with the behavior of the first signature that would solve the problem.

But a nice feature of async / await is that it makes dealing with async exceptions pretty similar to dealing with sync exceptions, and it would be unfortunate if the test framework could not accommodate that analogy. Another option is to change throws so that it returns a Promise even on synchronous exceptions:

export interface ThrowsAssertion {
    (
        fn: () => any,
        expectations?: null | ErrorExpectations,
        message?: string,
    ): Promise<any>;

    /** Skip this assertion. */
    skip(thrower: any, expectations?: any, message?: string): void;
}

It could be slightly annoying to have to use async code to wait for synchronous
exceptions in cases where the test needs to reference the exception that was
thrown. But it would make the type ambiguity problem go away.

[hallettj]

Similarly, we support tests that return thenables, rather than promises. Typing these "promise-like" objects is also more difficult than expected (#1778). At the very least we could change the typings to expect a Promise.

I think this is reasonable. The majority of cases will be async test functions, which return a native promise. Third-party promise implementations will likely have an option to convert a third-party promise into a native one.

Unfortunately Flow does not provide a promise interface - the definition for the native Promise is a class, because promises can be constructed, and Promise has static methods. It would be nice if they shipped a Thenable interface compatible with the native promise definition.

On the other hand I think that the change I made in #1778 is a reasonable solution. I did see the note that you pointed out from a76d462:

This seems to have broken using async-await

The whole point of the change was to fix uses of async-await, and it works in my testing. I don't know what is going on in @jamiebuilds' case.

I don't quite think it's worthwhile enforcing tests to return proper Promise instances rather than thenables, and properly typed promise implementations should match the Promise interface anyhow.

I tend to agree; but I think that both options are reasonable.

[novemberborn]

Wow, thanks for the detailed response @hallettj!

We just need to express that if the first argument is a function that returns a PromiseLike or ObservableLike then the return type is a Promise; otherwise the return type is not a Promise (which is expressed here as any but perhaps could be given as ?Error);

We could refine this to Error, however if the assertion fails it returns undefined. I may have typed it as any so you can cast it to an Error if appropriate without really having to worry about type safety in case of an assertion failure.

If we change throws to behave only according to the second signature, and create a new assertion called rejects with the behavior of the first signature that would solve the problem.

But a nice feature of async / await is that it makes dealing with async exceptions pretty similar to dealing with sync exceptions, and it would be unfortunate if the test framework could not accommodate that analogy.

Interesting. We're trying to keep the number of assertions to a minimum, while still covering most use cases.

Another option is to change throws so that it returns a Promise even on synchronous exceptions (…) It could be slightly annoying to have to use async code to wait for synchronous
exceptions in cases where the test needs to reference the exception that was
thrown. But it would make the type ambiguity problem go away.

An added complication is that when used with a promise / observable, you must await the assertion. It'd be weird to not mandate that for synchronous exceptions.

That said, perhaps the current behavior is also confusing. We could make t.throws synchronous, and add an asynchronous t.errors() that still works with synchronous errors. It'd be cumbersome to port existing tests though we can make sure to print helpful error messages.

Unfortunately Flow does not provide a promise interface - the definition for the native Promise is a class, because promises can be constructed, and Promise has static methods. It would be nice if they shipped a Thenable interface compatible with the native promise definition.

Would Flow's Promise class match say a Bluebird promise? I think that'd be sufficient.

I don't quite think it's worthwhile enforcing tests to return proper Promise instances rather than thenables, and properly typed promise implementations should match the Promise interface anyhow.

I tend to agree; but I think that both options are reasonable.

Yea I'm torn on this. I don't want to force users to await a Bluebird promise, basically.

---

To recap, @hallettj are you saying that if we had an t.errors() that always returns a promise, we can overload that interface to accept observable-like things, streams, async iterators, regular functions, and promises? And then we'd have t.throws() always be synchronous?

[sindresorhus]

It feels weird having to add/change APIs because of Flow/TS limitations...

[sindresorhus]

tl;dr: I think that the simplest option is to split throws into multiple assertions: throws for functions that should throw a synchronous error, rejects for functions that should return a rejected promise,

I've considered this before, but with async/await the naming becomes unnatural, as you usually use the throw keyword in async functions instead of Promise.reject(), so it doesn't really seem like it's rejecting.

[hallettj]

We could refine this to Error, however if the assertion fails it returns undefined. I may have typed it as any so you can cast it to an Error if appropriate without really having to worry about type safety in case of an assertion failure.

So it could be Error | void to indicate that the value may or may not be a defined error.

An added complication is that when used with a promise / observable, you must await the assertion. It'd be weird to not mandate that for synchronous exceptions.

I think it would work to fail the test synchronously if an error is thrown synchronously, but to require the user to wait on a promise to examine the error that was thrown regardless. In any case that would make the types work out.

Would Flow's Promise class match say a Bluebird promise? I think that'd be sufficient.

Yes, because Bluebird's promise is a subclass of the native Promise. Or at least it is declared to be a subclass in the type definitions on flow-typed.

To recap, @hallettj are you saying that if we had an t.errors() that always returns a promise, we can overload that interface to accept observable-like things, streams, async iterators, regular functions, and promises? And then we'd have t.throws() always be synchronous?

Yes, exactly - if we can limit the assertion type to a single signature then that will fix the problem. If the return type is always the same for a given assertion then we can use one signature with type unions for argument positions to accept various observable-like arguments, various different matchers, etc.

[sindresorhus]

Similarly, would it be a terrible bother if our typings were stricter on promises?

I would be fine with this if we used a Object.prototype.toString.call(promise) (this would allow libs to set Symbol.toStringTag) type check and not instanceof.

If you're using observables or generators, how would you feel if we'd remove first-class support?

I don't use either, but I've seen a lot of people use the observable support in their tests implementation. I'm fine with removing observable support from t.throws though. It always felt out of place. And I think we should definitely remove support for generator functions in the test implementation. It's really just a leftover from the early days when generators were a popular alternative to async/await.

[hallettj]

Similarly, would it be a terrible bother if our typings were stricter on promises?

I would be fine with this if we used a Object.prototype.toString.call(promise) (this would allow libs to set Symbol.toStringTag) type check and not instanceof.

FWIW I think that the permissive PromiseLike interface as of #1778 is the way to go. It describes the minimal shape of a promise, and matches up with the runtime checks that the is-promise package performs.

I think that trouble in front of us is not due to descriptions of promises, but is due to Flow not having enough information to choose between multiple function signatures in some cases.

If you're using observables or generators, how would you feel if we'd remove first-class support?

I don't use either, but I've seen a lot of people use the observable support in their tests implementation. I'm fine with removing observable support from t.throws though. It always felt out of place. And I think we should definitely remove support for generator functions in the test implementation. It's really just a leftover from the early days when generators were a popular alternative to async/await.

This all seems sensible to me. Especially the bit about generators.

I use a lot of reactive streams that are in theory compatible with the Observable interface. But the Observable standard is not in a solid place right now. Every project has to create its own version of the interface. I am more likely to use custom assertions that apply to the particular stream implementation that I am working with.

[jamiebuilds]

It feels weird having to add/change APIs because of Flow/TS limitations...

It's definitely possible to type flow correctly here. This sounds like a separate API decision to me

[novemberborn]

We could refine this to Error, however if the assertion fails it returns undefined. I may have typed it as any so you can cast it to an Error if appropriate without really having to worry about type safety in case of an assertion failure.

So it could be Error | void to indicate that the value may or may not be a defined error.

@hallettj yes that's worth another shot. I can't remember why I did not do that.

An added complication is that when used with a promise / observable, you must await the assertion. It'd be weird to not mandate that for synchronous exceptions.

I think it would work to fail the test synchronously if an error is thrown synchronously, but to require the user to wait on a promise to examine the error that was thrown regardless. In any case that would make the types work out.

@hallettj the problem is that when used asynchronously you must await, regardless of whether you want to examine the error. I don't like having an asynchronous way of getting the error that you may or may not need to await.

I'm fine with removing observable support from t.throws though. It always felt out of place. And I think we should definitely remove support for generator functions in the test implementation. It's really just a leftover from the early days when generators were a popular alternative to async/await.

@sindresorhus great, let's do that.

I think that trouble in front of us is not due to descriptions of promises, but is due to Flow not having enough information to choose between multiple function signatures in some cases.

@hallettj does this problem go away if we remove ObservableLike?

tl;dr: I think that the simplest option is to split throws into multiple assertions: throws for functions that should throw a synchronous error, rejects for functions that should return a rejected promise,

I've considered this before, but with async/await the naming becomes unnatural, as you usually use the throw keyword in async functions instead of Promise.reject(), so it doesn't really seem like it's rejecting.

@sindresorhus even with the type problem resolved, we still have t.throws() sometimes returning a promise and sometimes not. Perhaps it's fine. Or perhaps we could have t.throwsSync() that only works with synchronous errors, and t.throws() which always returns a promise that must be awaited?

It feels weird having to add/change APIs because of Flow/TS limitations...

It's definitely possible to type flow correctly here. This sounds like a separate API decision to me

@jamiebuilds if you have some time to help out here that'd be great. Does master have a problem now with async/await? I don't think @hallettj can reproduce that.

[hallettj]

I think that trouble in front of us is not due to descriptions of promises, but is due to Flow not having enough information to choose between multiple function signatures in some cases.

@hallettj does this problem go away if we remove ObservableLike?

No; even if there are only two options, a function that returns a promise (() => PromiseLike<any>), or a function that throws an error synchronously (() => any), Flow does not have enough information to choose one signature because the second option will be applicable in every case where the first is applicable. What we would need to disambiguate would be a type like () => any type that is not a promise. Unfortunately Flow does not have a way to express that.

[novemberborn]

even if there are only two options, a function that returns a promise (() => PromiseLike), or a function that throws an error synchronously (() => any), Flow does not have enough information to choose one signature because the second option will be applicable in every case where the first is applicable. What we would need to disambiguate would be a type like () => any type that is not a promise. Unfortunately Flow does not have a way to express that.

@hallettj oh so you're saying the problem is the any return type?

I remembered more about why I typed it like that, but we can reconsider:

Technically, the assertions can return undefined if they fail. But practically we don't really care about runtime crashes after an assertion failure, so we could always type as Error.

Can Error still be upcast to a specific error instance? If not, that would've been another reason for using any. But perhaps we can use a generic that defaults to Error, so users can type their assertion as appropriate?

If all that works out then we could retain the current synchronous / asynchronous t.throws() behavior, but with better typing. Which perhaps @jamiebuilds was referring to?

[hallettj]

even if there are only two options, a function that returns a promise (() => PromiseLike), or a function that throws an error synchronously (() => any), Flow does not have enough information to choose one signature because the second option will be applicable in every case where the first is applicable. What we would need to disambiguate would be a type like () => any type that is not a promise. Unfortunately Flow does not have a way to express that.

@hallettj oh so you're saying the problem is the any return type?

I'm sorry; the problem is not the return type of t.throws() - it is the return type of the function that is given to t.throws(). If that is restricted to something other than any or mixed then users would get errors that they probably do not expect.

[novemberborn]

I'm sorry; the problem is not the return type of t.throws() - it is the return type of the function that is given to t.throws(). If that is restricted to something other than any or mixed then users would get errors that they probably do not expect.

Oh I see. That does leave us in a pickle. @jamiebuilds any ideas?

@sindresorhus any thoughts on a t.throwsSync() that never returns a promise (and thus fails with async errors) and a t.throws() that works like our current one except it always returns a promise that must be awaited?

@hallettj any thoughts on typing the assertions to return an Error / Promise<Error>, and perhaps t.throws<MyExpectedError>()?

[novemberborn]

I've opened #1866 and #1867 to remove the generator and observable cruft.

The remaining issue is that we've overloaded t.throws() & t.notThrows() to return a promise, if they're called with one, or with a promise-returning-function. We do not return a promise when called with a regular function, in which case the function argument is typed as () => any. Flow cannot distinguish between these two scenarios.

I think the cleanest solution is to make t.throws()/t.notThrows() always return a promise. This way the return value of the function does not matter, simplifying our type definitions. It would still work with functions that throw synchronously, however the t.throws() & t.notThrows() assertions must now be awaited on. This is a breaking change.

We can add t.throwsSync() and t.notThrowsSync() for functions that must not return a promise. We can't type this restriction, but we can fail the assertion if a promise is returned.

I also want to consider typing the return value as Promise<Error> / Error respectively. I'll have to see if these can be upcast, or else be specified as a generic in the assertion itself. This may preclude a resolution to #1841, or if using generics perhaps not.

[sindresorhus]

It would still work with functions that throw synchronously, however the t.throws() & t.notThrows() assertions must now be awaited on. This is a breaking change.

This would be really awkward for synchronous tests.

I think we should keep t.throws() synchronous, but remove promise support and introduce a t.throwsAsync() for that instead. Test implementations are synchronous by default, and I think t.throws() should be that too. It's also what people would expect coming from other assertion APIs.

I also want to consider typing the return value as Promise / Error respectively.

👍

[novemberborn]

Fair enough.

Now that we've removed observables, is there a case for a t.rejects() that fails if passed a function that throws synchronously? Rather than t.throwsAsync().

[sindresorhus]

is there a case for a t.rejects() that fails if passed a function that throws synchronously? Rather than t.throwsAsync().

Not sure I understand. Fail in what sense? Fail the test or process.exit() kinda fail? And how is it different from t.throwsAsync()?

[novemberborn]

I meant instead of throwsAsync():

await t.rejects(Promise.reject(new Error()) // passes
await t.rejects(async () => { throw new Error() }) // passes
await t.rejects(() => { throw new Error() }) // fails test

[sindresorhus]

The reason I prefer the naming throwsAsync over rejects is that most of the time I use async functions and there you throw too.

[sindresorhus]

await t.rejects(() => { throw new Error() }) // fails test

What's the use-case for supporting this? I really think if we split up the methods we should not allow synchronous stuff in t.throwsAsync nor asynchronous stuff in t.throws.

[novemberborn]

await t.rejects(() => { throw new Error() }) // fails test

What's the use-case for supporting this? I really think if we split up the methods > we should not allow synchronous stuff in t.throwsAsync nor asynchronous stuff in t.throws.

That's what my example does. A synchronous throw is not allowed in throwsAsync(), and thus the test fails.

Which is why I suggested possibly going for rejects, but I agree that you'd still throw in an async function, so I'm happy with throwsAsync.

[sindresorhus]

@novemberborn Ah, I totally misunderstood you. Yeah, then we agree.

[novemberborn]

Tracking the remaining work in #1893.