-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Promise / Async / Await Inconsistencies / Issue #5294
Comments
I think this relates to your return being Promise<{}> when it should be Promise<{[key: string]: any}> |
Nope, in my original implementation I use a variant of that. Also, using any = lost coverage. Our projects have a fairly strict 100% coverage rule minus a few edge cases. It simply has to do with union handling and refinement issues. It mostly makes sense what it is doing and why, but it is a problem in terms of user confusion and how it is reported. Promises, being asynchronous, tend to introduce a lot of complexity to such things anyway. |
Running into this issue more and more as I build things up. It appears that if you return ANY object through a Promise w/ For example, I have a class which directly returns: send = async (...args: Array<any>): Promise<PubChan$EmitResponseRef> => { ... } When I try to use it: async function collectResults() {
const ref = await chan.emit('bar', 'foo').send();
const { results } = ref;
if (Array.isArray(results)) {
for (const result of results) {
console.log('Result: ', result);
}
}
} It will cause errors. Which appear to be the same issue, it is returning Whereas if I do not use await, no errors: It can be fixed in this situation by simply changing // we use this signature when typing await expressions
declare function $await<T>(p: Promise<T>): T; However, I can only imagine this is going to break things in many other situations.
|
@bradennapier I believe i'm experiencing the same thing, not sure if the examples here help, https://stackoverflow.com/questions/47817511/flow-errors-on-promises |
I'm surprised that this issue has so few comments, the patchy support for await/async is my number one frustration with flow. |
Cant say I've ran into the issues described here, and I'm using Promises everywhere in my code. There must be something specific being done to trigger the supposed issue, or there is a misunderstanding either one. Flow's typedef is correct, see in console: (async function () { console.log(await 4); })();
// prints 4 My only frustration I run into is when I do something wrong and the error message isn't as clear as to what my error is, or where the error sourced. IE i once had const foo = await bar; when i needed const [foo, err] = await bar; then when I used foo.something - it triggered errors in other confusing locations. Flow knew something was wrong, but didn't tell me exactly why. But ive yet to write valid code working with promises that flow complained about. |
I am getting the same behavior, when awaiting a method which returns a promise, the resultant type is Moreover applying @bradennapier definition of await does fix the return type to no longer include Promise, but it does as he points out break other aspects of the typing. For now we are working around this by applying the type explicitly at the callsite, i.e. let foo = await getFoo()
// foo: ExpectedType | Promise
let foo: ExpectedType = await getFoo()
// foo: ExpectedType |
This is still completely broken, even the most basic tryflow example doesn't work as of 0.79.1, including attempts with @rt2zz 's workaround or by wrapping the promise locally. |
@rakelley that try flow example is wrong any async method MUST!!! return a Promise |
Okay, I didn't expect to have to explicitly type the implicit wrapper, but regardless that just shifts the problem one call level up ( |
Looking back at the original complaint, i think this whole thing is without merit. If your method is marked async, it returns a promise.... no ifs ands or buts. Try this in the REPL: (async function() {})() You will get a Promise. If you are expecting to write async function getFoo(): Promise<Foo> {
return new Foo();
}
const foo: Promise<Foo> = getFoo();
const foo: Foo = await getFoo(); That is the only valid code for async functions and it works just fine in flow. This is where it gets dirty: let cached: Foo;
function getFoo(): Promise<Foo> | Foo {
if (cached) {
return cached;
} else {
// yay dirty race condition bug here too!
return new Promise((resolve, reject) => {
fooProvider().then((foo) -> {
cached = foo;
resolve(foo);
}).catch(reject);
});
}
}
const maybeFoo: Promise<Foo> | Foo = getFoo();
// We MIGHT of been cached, maybe not? let's guarantee we have a foo:
const foo: Foo = await Promise.resolve(maybeFoo); You now have a function with mixed return types. You really should avoid this.... just mark the getFoo method async and now your function consistently returns Promise @rakelley with async keyword, you will never ever have Promise | Foo, only if you do this 2nd example where you don't use the async function keyword. |
I take back part of my last statement. I see the OP is mentioning something else, but I can't say I've ran into that scenario. But as I said in January, Flow's typedef is correct. I tried the OP's suggested broken code and fixed the return type of the test/foo method and it passes without error: |
Anyone getting here through google, the solution for me was that i needed to define the type of return in the function i was await-ing:
Given that the method was to return an Object from the promise. This took away the errors for me. |
Running into this situation as well using node-pg. query.js // @flow strict
import { Pool } from "pg";
import type { ResultSet } from "pg";
const pool = new Pool();
export default (text: string, params: Array<any>): Promise<ResultSet> =>
pool.query(text, params); test.js // @flow strict
import query from "./query";
export async function runQuery() {
const test = await query("SELECT * FROM TABLE;", []);
return test;
} Flow seems to think |
Yep you need to provide the type in this case (on the result var directly) and it will solve the issue. Not ideal clearly but it has a workaround at least. |
Also to those of you mentioning you can’t recreate it in flow try - as I pointed out it ONLY HAPPENS when you import from another file. In the same file it seems to be fine. |
I can confirm, I have similar errors only when multiple files are involved. |
I think I have run into this issue. File1.js: const getThings = async (): Promise<Array<SomeType>> => { ... };
export { getThings }; File2.js: import { getThings } from 'File1';
const doStuffWithThings = async (): Promise<Array<SomeOtherType>> => {
const things: Array<SomeType> = await getThings();
...
things.forEach(thing => { ... });
...
} Flow complains that |
I think it would be really helpful if somebody put up a gist with a complete example that exhibits the unexpected behavior. Unless I missed something, all I've seen on this issue are snippets from actual codebases, or other partial examples. I've seen what I think is the same issue during my own use of Flow. It's always been a legitimate error, though, which additional type annotations helped to reveal. If I'm correct, this issue is about opaque error messages, and not about Flow erroring when it should not. A concrete example would help determine whether I'm correct. |
Thanks @nmote ! I believe you are 100% correct, the issue happens if you have any accesses on the value which are not in the expected return type in the I believe the best solution would be for In all my situations so far it does end up being an error on my part - although sometimes very hard to find as it can be a random property access at nearly any point after that since flow is generally fairly smart at these things. Another key point is it only appears to happen if you are importing the |
The typedef signature is correct and does not need to be changed. @bradennapier I state this in hopes that it makes what the true nature of this bug is, to be more clear, as you tacked on the note about 'another file' as 'another' key note, but that is the only key note that matters here.
My assumption is that it the passed argument to the parameter is able to 'accurately' identify it matches the Promise signature and reduces to only that as a match, but the assurance of cross-file type validation is treated differently and doesn't reduce to only one. @nmote does that help? Sorry I don't have time to make a gist atm though. |
@nmote I think you're right that this about 'opaque error messaging'. In all my attempts to make a small repro case, See https://gist.github.com/akrieger/f1aa74fd5b5819cef96c100fc034d63b for an example. I've commented in index.js the place where the error is introduced and how to resolve it, and described how that changes flow's behavior. |
@akrieger we had a similar issue with this "red herring" error message, and Our example minimally was this: // file1.js
type Response = {
propA?: boolean,
propB?: string,
}
export async function anAsyncFunction(input: any): Promise<Response> {
// stuff
} // file2.js
import { anAsyncFunction } from './file1.js'
async function doSomeWork() {
...
const { propA = false, propB } = await anAsyncFunction('some input')
...
const templateLiteral = `here's a string with ${propB} interpolated`
...
} When running
However, after running
The fix was to either add a sensible default when destructuring the response, like: const { propA = false, propB = '' } = await anAsyncFunction('some input') or use the const templateLiteral = `here's a string with ${String(propB)} interpolated` I think this is a bug bc |
@kweiberth |
Hi @silvenon that makes a lot of sense. I actually mistyped on the last line and I've since updated it. The issue I see here is that the error message that flow originally gave was that |
I can confirm that declaring the expected return type on the variable seems to clear it up. i.e. do this:
not this:
|
@cappslock hm, that doesn't seem right, are you sure that you didn't lose the type for |
This is a silly issue to have :) |
This seems easy to fix, just replace declare function $await<T: Promise<any>>(p: Promise<T>): $Call<typeof $await, T>;
declare function $await<T>(p: Promise<T>): T;
declare function $await<T>(p: T): T; This is also just better definition overall, because it makes |
Well, with mine definition, Flow tests on |
Also it breaks another test, well that's too bad |
Thank you @kweiberth! the issue was indeed that a type was being coerced a few lines after... still the error message is super confusing. |
I was able to simplify the the reproduction to a single I've tried to demonstrate that this error
|
ignore my last reply, i re-interpreted it the report. ultimately its because you forced flow to pick promise return BECAUSE the left hand side is wrong Only way i can see fixing this to identify the users error is to explicitly look for explicit Promise Left Hand Side types of an await. |
I've added https://gist.github.com/terite/47e5d725bae07723fb11ea5b4a470a65#file-two-js |
This appears to have been fixed! When I run a Full error output
OTOH the issue reproes if I use v0.141.0, the latest release. In fact, that commit and v0.141.0 have just a very few differences: which, other than the test suite, are entirely that v0.141.0 has reverted some changes that look related to "types-first". So it appears that those changes (which were reverted just for that release) fix this issue. (Or perhaps more accurately: that types-first fixes this issue, but reverting those changes meant types-first doesn't apply here.) Hopefully that means that a future release will include those changes, and will fix this bug. |
Checked example from @terite with Flow 0.159 and looks like this is now fixed. |
Ok so this one took me forever to simplify to the point we can see what is happening here, although it is still a pretty confusing bug which is already extremely nasty. It only shows itself in very specific situations and causes confusing issues which are often masked as different bugs completely!
This issue is definitely avoidable - but the end result shows that there is a need to handle this situation in a better way.
The Problem
When using async/await there are times that it ends up returning the
Promise
class as the return type. Meaning that in order to use the return value in this case as an object of any form, you need to also refine!val instanceof Promise
.Many times it doesn't actually show that this is happening. I haven't been able to refine the specific cases I have run into but it ends up happening when this error compounds onto others and Flow decides to only show the errors that this causes rather than the underlying issue itself.
Overview
Essentially the end result looks like this (note the examples here are all over simplified)
We are programming and all is well, we inspect return types and we like what we are seeing:
Then all of the sudden things get problematic. We start seeing errors about Promises and unions:
Upon inspection, if we are lucky enough that this is the error that actually shows, we can see that the following has happened:
The Analysis
It is likely due to the following logic within the definition of
$await
which is also mirrored through withinPromise
(although it doesn't appear to occur whThis kind of logic is used throughout and the problem seems to come in when refinement causes it to return the second value within the union rather than the first (and the result is
Class<Promise>
)Re-Creation
So the issue seems to occur when a few parameters are met:
this
, another object, etc) with an annotation.then from
./test.js
, return any kind of promise.Closing
While the reason for this becomes obvious with this simple example, the issue ends up showing itself in extremely nasty and hard to debug situations.
In this case it is because the annotation returning the promise clearly only shows
{}
and doesnt perfectly match the given object.However, this behavior only occurs when all of the given conditions are met and creates inconsistent handling. Using
.then()
will never show this bug (that I have experienced) so every time that it has occurred it has ended up been very very confusing.It took awhile to track down these specifics.
So while it isn't really a bug, I have seen quite a few examples of people reporting issues that are likely related to this specific issue.
It should probably either be made to be much more consistent or to provide an error which is easier to understand what and why the issue is happening.
In my case it was happening when trying to type objects which will be highly variable and trying to maintain coverage as much as possible.
The text was updated successfully, but these errors were encountered: