Make the graphqlExpress middleware return a Promise

[cuzzlor]

To allow consumers to implement post-execution middleware by wrapping the function, resolving the promise and calling next(), without a breaking change inside the implementation (e.g. calling next())

See #462

e.g. this can now be done

const graphqlHandler = graphqlExpress({ schema: myGraphQLSchema });

app.use(
  '/graphql', 
  bodyParser.json(),
  (req, res, next) => {
    Promise.resolve(graphqlHandler(req, res, next)).then(() => {
      next();
    }).catch((e) => {
      next(e);
    });
  },
  (req, res, next) => {
    // post process e.g. clean up request-scoped resources here
  }

TODO:

• Update CHANGELOG.md with your change (include reference to issue & this PR)
• Make sure all of the significant new logic is covered by tests
• Rebase your changes on master so that they can be merged easily
• Make sure all tests and linter rules pass

[apollo-cla]

@cuzzlor: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[abernix]

While this might seem harmless, this will cause problems for all existing implementations. As of Express 4, middleware should still not be returning Promises. This is being tracked in expressjs/express#2259 and is planned for Express 5. In your example above you do change graphqlExpress to return a Promise, but as you can see, it's still necessary to return a traditional callback to Express' router via express().use.

Most notably, the current pattern employed by most implementations of apollo-server-express would look like this:

app.use('/graphql', graphqlExpress({ schema }));

The change submitted here would require that graphqlExpress be wrapped in a traditional callback with the standard middleware signature ((req, res, next)).

I appreciate the idea here, but I think it's important for us to think about this carefully. @fjcero opened an issue for discussion in #953, and I think it's worth having that discussion there first, especially something which is categorically a major breaking change like returning a Promise instead of a callback on a public API.

[cuzzlor]

I'm confused what you mean by causing problems for all existing implementations.

The prior implementation internally called an async function but did not await it and returned void, making the underlying async function un-awaitable.

Surely existing implementations will be unaffected by the function returning the promise unless the consumer changes code to explicitly await resolution of that promise?

One of the reasons I thought that would be OK is that I originally used that type of custom wrapper awaiting promise resolution to have post execution middleware run when using the express-graphql middleware, which returns Promise<void>. In that case it seems very infrequent that consumers choose to await it, but the option is available. https://github.com/graphql/express-graphql/blob/master/src/index.js