Express example misuses promises

[cloudlena]

I have a project which follows the "Simple Setup" example given in this project's README: https://github.com/mastertinner/graphql-jsonplaceholder/blob/master/src/index.ts#L19

I have configured eslint to run the recommended TypeScript linting rues. The code throws a linting error because the graphqlHTTP function returns a promise which is not expected by express at that point because they are expected to return void as can be seen here:

error  Promise returned in function argument where a void return was expected  @typescript-eslint/no-misused-promises

Is this expected or am I doing something wrong? Do we need to update the TypeScript definitons respectively?

[MatrixFrog]

I noticed the same thing. Did a little bit of digging and I found this:

Starting with Express 5, middleware functions that return a Promise will call next(value) when they reject or throw an error. next will be called with either the rejected value or the thrown Error.

https://expressjs.com/en/guide/writing-middleware.html

While it doesn't say so explicitly, I think the idea is that if a middleware function returns a promise that resolves rather than rejects, it's equivalent to calling next() with no argument (indicating success). And if you look at the asynchronous middleware function returned from graphqlHTTP, it never calls next at all, it just throws (which means the returned promise will reject) or reaches the end without throwing (which means the returned promise will resolve to undefined)

express-graphql/src/index.ts

Line 189 in 28e4c29

return async function graphqlMiddleware(

So, it looks like express-graphql is designed for this new behavior in Express 5, while I'm still on Express 4 (as are you, I'm guessing). Perhaps they can release two separate versions for use with Express 4 and 5. In the meantime, you could wrap it and call next; something like this:

app.use("/graphql", (req, res, next) => {
  const promise = graphqlHTTP({
    schema,
    rootValue,
  })(req, res);
  promise.then(() => next()).catch(err => next(err));
});

but that feels kind of clunky so perhaps it's easier to just suppress the eslint warning

[MatrixFrog]

Thinking about this a little more, I don't really understand why this package is designed to be used as middleware rather than as a handler.

[DiegoRBaquero]

Just pointing out that the handler itself doesn't need to then to next else it will also trigger a 404 when the response is already set. This should be the right way

app.use("/graphql", (req, res, next) => {
  graphqlHTTP({
    schema,
    rootValue,
  })(req, res).catch(next);
});