Move 'doNotWaitForEmptyEventLoop' middleware from 'after' step to 'before'?

[middyjs]

Move 'doNotWaitForEmptyEventLoop' middleware from 'after' step to 'before'?

[vladholubiev]

@lmammino 🤔 🤝

[lmammino]

Thanks @vladgolubev, that's probably a good idea. I think we have to make this middleware slightly smarter to handle this case (and more).

The original idea of having it in the after phase was to make sure that the user does not override it by mistake, but we didn't think it might have affected error cases.

One idea could be to set the callbackWaitsForEmptyEventLoop to false in both phases (before and after) and probably we should do it also in the onError phase.

Also, we could assume the before phase is mandatory and the after and onError phases could be switched off through config.

Using these ideas, this can be a first draft of the new version:

module.exports = (opts) => {
  const defaults = {
    runOnAfter: true,
    runOnError: true
  }

  const options = Object.assign({}, defaults, opts)

  const setFlag = (handler, next) => {
    handler.context.callbackWaitsForEmptyEventLoop = false
    next()
  }

  return ({
    before: setFlag,
    after: options.runOnAfter ? setFlag : undefined,
    onError: options.runOnError ? setFlag : undefined
  })
}

@dkatavic, what do you think?

[lmammino]

That makes totally sense @dkatavic. Thanks!

Probably we should also set more efficient defaults like:

const defaults = {
    runOnBefore: true,
    runOnAfter: false,  // user code (or other middlewares) might reset the flag, but no extra function is executed
    runOnError: false  // user code (or other middlewares) might reset the flag, but no extra function is executed
  }

@vladgolubev, do you want to take care of submitting a PR with this? 😉

[dkatavic]

Good catch @vladgolubev . I like the draft @lmammino , just I would like to make the middleware as configurable as possible, so can we add runOnBefore option too?