"Thenables" does not necessarily means promise objects - Failing execution on "badly-formed" promises

[rricard]

Some libraries (such as very old mongoose versions) does not use native promises and use an old promise system that for instance won't support the .catch statement. those things are typically "thenable" but not "catchable". The issue here is that GraphQL execution engine considers everything "thenable" as "catchable".

This triggers the following stacktrace:

TypeError: completed.catch is not a function
      at completeValueWithLocatedError (node_modules/graphql/execution/execute.js:493:29)
      at completeValueCatchingError (node_modules/graphql/execution/execute.js:467:21)
      at resolveField (node_modules/graphql/execution/execute.js:440:10)
      at node_modules/graphql/execution/execute.js:219:20
      at process._tickDomainCallback (internal/process/next_tick.js:129:7)

This "bug" isn't really one if you use "real" promises everywhere but is one if you have to deal with legacy systems not quite updated yet.

I will provide a simple fix to this issue if you think it's worth fixing.

[rricard]

Note that you can fix that in your resolver easily if you are in this situation by wrapping the dysfunctional thenable into a real promise:

() => Promise.resolve(badThenable)

I still think a fix in the graphql-js codebase would be great since the error is hard to understand when it happens. Moreover, checking the availability of .catch seems always interesting to do before actually using it!

[denvned]

BTW, the original Promises/A+ standard does not define .catch(onRejected). It was added by ES6 Promise API and some other promise implementations as a shortcut for .then(undefined, onRejected).

[leebyron]

Ah this is definitely unintentional and should be considered a bug. graphql-js should only be relying on the more minimal definition of Promises and not relying on catch.

[rricard]

@leebyron you fixed that too fast for me ;) Thanks, this will help!