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
Use PromiseLike for isPromise
type guard
#3243
base: main
Are you sure you want to change the base?
Conversation
|
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.
Amazing 🎉 this type guard makes more sense.
I would suggest renaming the function to isPromiseLike |
Actually I wondered if I should rename |
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 agree with @yaacovCR we need to be clear with names.
So please rename all the stuff (function, filename, tests, etc.).
Also please note that we considering dropping support for non-standard Promises in the next release (17.0.0) to make our codebase simpler and more modern.
Can you please provide context on why you're using Bluebird?
Is it for legacy reasons or Bluebird still has some advantages over native promises in 2021?
That said happy to merge your PR into 16.x.x line.
@IvanGoncharov As long as an object has a I think keeping the return typeof value?.then === 'function';
If I had to guess, it'd probably be legacy reasons / external libraries that use it, though it still looks like there's maybe still some performance wins for Bluebird with parallel promises For example Knex, which I created back in 2013 when Promises weren't part of core JS, uses Bluebird and leans in pretty heavily on utility methods like I had a similar request to use |
I'm actually using aigle, which has convenient methods to control concurrency (like eachLimit, etc). Its promise-like object is almost adaptable to native Promise, but just its type of TypeScript is not native Promise. For example: aResolver: () => {
return Aigle.eachLimit(...) // return type is Aigle
} Actually it can be rewritten as below, and then it's no longer a problem. aResolver: async () => {
return await Aigle.eachLimit(...) // return type is native Promise
} @tgriesser Thank you for adding explanation and the question for clarification. It might be a big breaking change if resolver functions must return native Promise value in 17.x, but I'd like to listen the maintainer's opinions. |
@tgriesser We actually removed all use of Bluebird from knex last year, it runs completely on native Promises now. Generally speaking, Bluebird has less consistent performance (under heavy load, at best it performs comparably to native Promises, at worst significantly worse) and is not recommended for non-legacy projects. |
@qsona @kibertoad @tgriesser Thanks for feedback 👍 Here are couple of reasons I was thinking about dropping support for non-Native promises:
const schema = buildSchema(`
type Query {
lastTransaction: Transaction
}
type Transaction {
then: String
}
`);
const rootValue = {
lastTransaction() {
return new Transaction();
}
};
class Transaction {
then(...args) {
if (args.length === 1 && typeof args[0] === "function") {
throw new Error("Oops, graphql-js called us thinking we are promise :(");
}
return "<some datetime>";
}
}
const result = execute({
schema,
rootValue,
document: parse(`
{
lastTransaction {
then
}
}
`)
});
console.log(result); Not the most critical issue in the world but pretty annoying especially for tools/services where clients can upload arbitrary schemas.
Would be great to hear your thoughts on these points. |
Point two is true only in a qualified sense, your schema can have a field with name then, but, if so, you cannot use class based resolvers for at least just that field… |
@yaacovCR You are right but it's a very non-obvious limitation, especially for tools and libraries that generate a schema from some input. For such it means they can't use class-based resolvers at all. It's a general issue with |
I agree unfortunate that there is not a well defined symbol to declare an object to be promise like. I think we should advocate for one. In the meantime, although I acknowledge your theoretical concern, we have an users were asking for PromiseLike support to be retained, and I do not see at least any recent issues related to the difficulties with the field name then. Automatic schema generation tools should be able to automatically handle this with an automatic workaround, presumably modifying the default merged resolver. Maybe anybody currently actually experiencing problems with fields named then could weigh in? |
Wow, that's a very interesting edge case, I'd never considered that one. That does remind me of the similar case where you can't return an const schema = buildSchema(`
type Query {
lastError: Error
}
type Error {
message: String
}
`);
const rootValue = {
lastError() {
return new Error('some-message');
}
};
const result = await execute({
schema,
rootValue,
document: parse(`
{
lastError {
message
}
}
`)
});
console.log(result); // { data: { lastError: null }, errors: ["Error: some-message"] } If we're considering dropping Also, would it be possible to check if there's field named |
@tgriesser Sadly not a solution 😞
Agree, it is similar. However, in the case of Error, it is at least done based on
@yaacovCR It's unrealistic to have such a workaround in every system that generates schema.
That's why I'm totally ok with merging this PR if the author address reviews comments. They also have a good argument there:
Also, a simple workaround is available |
I guess that it might impact more projects than you'd expect, mostly due to reasons like @qsona mentioned. I think the I've also seen many 3rd party integration SDK libs use Bluebird / Q for legacy reasons, and they're still pretty popular from a download count perspective: https://npmcharts.com/compare/bluebird,q,when,graphql If you do want to go down that path, the removal of |
I like the idea of deprecation warnings, especially since we have "dev mode" by default but clients can disable them by switching NODE_ENV to production. |
I agree with both @IvanGoncharov and @tgriesser, although the inability to return a PromiseLike object correctly in all cases and the inability to return an Perhaps a solution is to to take a page from GraphQLJava, which I seem to recall has a type GraphQLResult<T> =
| { error: Error; promise?: never }
| { promise: PromiseLike<T>; error?: never }; And the // If result is an Error, throw a located error.
if (result instanceof GraphQLResult && result.error) {
throw result.error;
} instead of this: // If result is an Error, throw a located error.
if (result instanceof Error) {
throw result;
} And the if (result instanceof GraphQLResult && result.promise) {
completed = result.promise.then((resolved) =>
completeValue(exeContext, returnType, fieldNodes, info, path, resolved),
);
} else if (result instanceof Promise) {
completed = result.then((resolved) =>
completeValue(exeContext, returnType, fieldNodes, info, path, resolved),
);
} else {
completed = completeValue(
exeContext,
returnType,
fieldNodes,
info,
path,
result,
);
} instead of this: if (isPromise(result)) {
completed = result.then((resolved) =>
completeValue(exeContext, returnType, fieldNodes, info, path, resolved),
);
} else {
completed = completeValue(
exeContext,
returnType,
fieldNodes,
info,
path,
result,
);
} Just thinking aloud. This could also work as two different types: |
Problem
The util function
isPromise
has a type guard to guarantee that return value isPromise
, but it's just checking if it hasthen
function, so I thinkPromiseLike
type is more suitable thanPromise
here.It seems that you invoke only
then
method carefully on promise-like object (example) , so usingPromiseLike
type might make it safer.Additional context
While I'm working on this issue, I'd like to clarify that the resolver functions can return PromiseLike value.