adding in the callback to express package

[chemdrew]

This is in relation to issue 462

Logic now matches the logic in the Restify package more closely.

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]

@chemdrew: 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/

[freiksenet]

This looks like a great addition. Thank you very much for this pull request.

[Friss]

It looks like this change causes #471 because it advances the middleware chain but because the response has been sent to the client it can't be modified.

Because this middleware is the endpoint that returns data I believe it shouldn't call next()

[chemdrew]

Any middleware happening post-execution should use the flag res.headersSent if it plans on modifying the response

[Friss]

I posted an example in the issue but I think the common use case that people will hit here is 404 handlers and error handlers.

This is a non obvious change that I'm sure a lot of people will hit especially when the express generator creates a 404 handler by default.

Here is my example of hitting this error https://gist.github.com/Friss/90604b5f53ad4bb38146a8f49c730c13

[chemdrew]

Fix as simple as this:

// error handler
app.use(function(err, req, res, next) {
  console.log('In error handler');
  // set locals, only providing error in development
  res.locals.message = err.message;
  res.locals.error = req.app.get('env') === 'development' ? err : {};

  // render the error page
  if (!res.headersSent) {
    res.status(err.status || 500);
    res.json({err});
  }
  next();
});

[rafgraph]

@freiksenet FWIW calling next() after returning data goes against the convention of how to handle 404's in express (as @Friss pointed out). Also, graphql-server-express has not (to my knowledge) implanted this in prior versions so I would consider it a breaking change, and definitely not a patch. It broke my app when upgrading from v1.0.0 (which was upgraded w/o issue from v0.8.5).

My suggestion would be to implement calling next() as an option that people can enable if needed.

[freiksenet]

I'm very sorry for breaking your code. This should have been a minor release, not patch.

There also seems to be disagreement if that's a best practice, I'll rather we figure out if it is, than make it an option. I'll revert the patch for now and will move a discussion back to the issue #462.