Use PromiseLike for isPromise type guard

[qsona]

Problem

The util function isPromise has a type guard to guarantee that return value is Promise, but it's just checking if it hasthen function, so I think PromiseLike type is more suitable than Promise here.

It seems that you invoke only then method carefully on promise-like object (example) , so using PromiseLike 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.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ qsona (5431332)

[saihaj]

Amazing 🎉 this type guard makes more sense.

[yaacovCR]

I would suggest renaming the function to isPromiseLike

[qsona]

Actually I wondered if I should rename isPromise to isPromiseLike, and I thought isPromise still makes sense. We can think that, in the context of graphql-js, Promise is defined as then-able object. PromiseLike is just a type name of TypeScript so I'm not sure isPromiseLike is a good name as a function name. (But of course I'd like to respect the opinion of maintainers)

[IvanGoncharov]

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.

[tgriesser]

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.

@IvanGoncharov As long as an object has a .then method which returns a value that passes the Promises A+ spec then it should be considered "standard". Can you elaborate on what you mean by dropping support for them?

I think keeping the PromiseLike type is the only requirement on graphql's end - and it's actually what the code for isPromise is doing, a duck-type check for being a PromiseLike, so I agree it'd be better named isPromiseLike:

return typeof value?.then === 'function';

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?

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 Promise.all.

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 Promise.map, Promise.props, Promise.using, etc. Ripping it out is no small feat and probably won't happen anytime soon (though @kibertoad's been driving the development of the project these days so I might be wrong).

I had a similar request to use PromiseLike in Dataloader awhile back for exactly the same reason: graphql/dataloader#146

[qsona]

@IvanGoncharov

Can you please provide context on why you're using Bluebird?

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.

[kibertoad]

@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.
Here's the latest analysis that I have seen on this topic: hazelcast/hazelcast-nodejs-client#559 (comment)

[IvanGoncharov]

@qsona @kibertoad @tgriesser Thanks for feedback 👍

Here are couple of reasons I was thinking about dropping support for non-Native promises:

1. It's very hard to not convert PromiseLike into promise, e.g. async function automatically do that
   https://www.typescriptlang.org/play?#code/IYZwngdgxgBAZgV2gFwJYHsLwBTAFwwAKATugLaogCmAMqgNZUA8S9E6A7hAHwCUBJcpVoNmrdl24wA3gCgYMYlWQJiWYAG4YsgL5A
   The same goes for Promise.all and other ECMAScript functions.
   So even if all of your resolvers returning Bluebird execution is still done using native promises.
   Don't test that setup but I suspect that such implicit conversions will hurt not only DX (e.g. it would way harder to trace and debug) but also overweight any performance benefits of using non-standard Promises.

2. Support for PromiseLike means we have strange behavior if the schema has any type with the then field.
   Example:

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.

3. If you still depend on some legacy code you can easily convert PromiseLike them into Promise using Promise.resolve.

Would be great to hear your thoughts on these points.

[yaacovCR]

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…

[IvanGoncharov]

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.
Also, the problem is not limited to the class-based resolver you can have schema base resolvers but your resolvers return some auto-generated object (e.g. ORM object with then method).

It's a general issue with *Like objects (another example is ArrayLike) since without using Symbol-based properties you can't reliably distinguish such objects.

[yaacovCR]

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?

[tgriesser]

Support for PromiseLike means we have strange behavior if the schema has any type with the then field.

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 Error from a resolver, which I think is a more common source of confusion:

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 PromiseLike, is this also something under consideration?

Also, would it be possible to check if there's field named then in the selectionSet, as a signal to skip automatic conversion?

[IvanGoncharov]

Also, would it be possible to check if there's field named then in the selectionSet, as a signal to skip automatic conversion?

@tgriesser Sadly not a solution 😞
Automatically generated schema can have both then and return promise for it at the same time.

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 Error from a resolver, which I think is a more common source of confusion:

Agree, it is similar. However, in the case of Error, it is at least done based on instanceof Error not just based on matching names. Also, it is useful for injecting errors in rootValue (useful in GraphQL proxies to remap error locations and paths). Also, it can be useful for bathing to error on individual items in batch.

Automatic schema generation tools should be able to automatically handle this with an automatic workaround, presumably modifying the default merged resolver.

@yaacovCR It's unrealistic to have such a workaround in every system that generates schema.

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.

That's why I'm totally ok with merging this PR if the author address reviews comments.
However, we need to clean out legacy stuff after the majority of the community migrated away from them (ArrayLike, dropping IE, etc.).
The question here is if we consider the PromiseLike as a legacy or not.
Bluebird usage is 100% legacy, as stated in their repo: https://github.com/petkaantonov/bluebird#%EF%B8%8Fnote%EF%B8%8F

They also have a good argument there:

Native Promises have been stable in Node.js and browsers for around 6 years now and they have been fast for around 3

Also, a simple workaround is available Promise.resolve.
So a question here is if we drop support for PromiseLike in v17 will it affect a significant number of active projects or not?

[tgriesser]

So a question here is if we drop support for PromiseLike in v17 will it affect a significant number of active projects or not?

I guess that it might impact more projects than you'd expect, mostly due to reasons like @qsona mentioned. I think the then duck-type check is a pretty well understood/used in JS vs. instanceof Promise

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 PromiseLike type & a deprecation warning / opt-in for the current duck-type check in 17 and outright removal in 18 seems like a decent path, rather than dropping the behavior completely in the next major. Just my 2c though.

[IvanGoncharov]

If you do want to go down that path, the removal of PromiseLike type & a deprecation warning / opt-in for the current duck-type check in 17 and outright removal in 18 seems like a decent path, rather than dropping the behavior completely in the next major. Just my 2c though.

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.

[yaacovCR]

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 Error from a resolver, which I think is a more common source of confusion:

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 Error object at all are not quite identical, they are similar.

Perhaps a solution is to to take a page from GraphQLJava, which I seem to recall has a GraphQLResult type object that may be returned for customization of the execution flow. Perhaps this object could have signature something like this:

type GraphQLResult<T> =
  | { error: Error; promise?: never }
  | { promise: PromiseLike<T>; error?: never };

And the completeValue flow would look like this:

  // 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 executeField flow would be something like this:

    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: GraphQLPromiseLikeResult, GraphQLErrorResult

[yaacovCR]

closed in favor of #4672