Middy: promise support documentation

[amwill04]

Hey

I dont know if I am completelty misusing this or not but given the following:

const middy = require('middy');


let count = 0

const handler = async function() {
  console.log(count)
  count++
  return {
        body: '{}',
        headers: {
            'content-type': 'application/json',
        },
        statusCode: 0,
    };
}

const asyncFunction = function() {
  return new Promise(function(resolve) {
    setTimeout(function(){
      resolve()
    }, 5000)
  })
}

const validator = function() {
  return {
    before: async function(handler, next) {
      console.log('start');
      await asyncFunction();
      console.log('end');
      next()
    }
  }
}

exports.handler = middy(handler).use(validator());

the count++ will be duplicated and provide different results:

START RequestId: 265b8c95-de83-4478-a821-e8ea7af078be Version: $LATEST
2019-10-17T15:55:56.289Z    265b8c95-de83-4478-a821-e8ea7af078be    INFO    start
2019-10-17T15:56:01.295Z    265b8c95-de83-4478-a821-e8ea7af078be    INFO    end
2019-10-17T15:56:01.296Z    265b8c95-de83-4478-a821-e8ea7af078be    INFO    0
2019-10-17T15:56:01.297Z    265b8c95-de83-4478-a821-e8ea7af078be    INFO    1
END RequestId: 265b8c95-de83-4478-a821-e8ea7af078be
REPORT RequestId: 265b8c95-de83-4478-a821-e8ea7af078be  Duration: 5068.85 ms    Billed Duration: 5100 ms    Memory Size: 128 MB Max Memory Used: 24 MB  Init Duration: 121.97 ms    

However, if you remove the next() call in the middleware it works as expected.

Am I misusing this or is this an issue?

[amwill04]

Possible due to this in the docs:

Middy allows you to return promises (or throw errors) from your handlers (instead of calling callback()) and middlewares (instead of calling next()).

Think this is a clear case to RTFM on my behalf. Can this be confirmed please?

[amwill04]

Trying to use the example in the docs

const asyncValidator = () => {
  before: async (handler) => {
    if (handler.event.body) {
      await asyncValidate(handler.event.body)

      return {foo: bar}
    }

    return
  }
}

handler.use(asyncValidator())

lambda will bug out. It seems return anything will cause it to fail. I can enhance the event body (what my middleware is actually wanting to do) by doing the following:

const validator = function() {
  return {
    before: async function(handler, next) {
      handler.event = {foo: 'bar'}
      return;
    }
  }
}

[lmammino]

If you use async you shouldn't call the callback. It will be automatically called when the function returns.

[amwill04]

Hey @lmammino thanks for that - was a clearly a RTFA moment.

I will close this but the second aspect I brought up are the docs just wrong or am I missing something? Thanks.

[amwill04]

The following is the only way I can achieve what I am looking for. I think this is the way...

// I use a try/catch and either set what I want on the event or
// I capture the error, set the status code as the message and pass it on to onError
// From here it sets the appropriate status code and returns

const validator = function() {
  return {
    before: async function(handler, next) {
      try {
        const foo = await asyncFunction();
        if (foo !== 'foo') {
          throw new Error('401')
        }
        handler.event = {foo: 'bar'}
        return
      } catch (err) {
        throw new Error('500')
      }
    }
    onError: function(handler) {
      const statusCode = parseInt(handler.error.message, 10);
      const response = {
        body: '{}',
        headers: {
            'content-type': 'application/json',
        },
        statusCode,
        };
        handler.callback(null, response)
    }
  }
}

There doesnt seem to be an obvious way to terminate early and return within an async function. This is the only way I have managed it. If this is correct can I raise a PR to put an example in the docs?

[lmammino]

This approach seems correct to me and I am super happy to have the documentation improved to make it easier for other users to get this right.

Thanks a lot for investing the time to clarify this so far.

If you proceed with a PR, please keep into account that we have a branch 1.0.0-alpha which will shortly become the new version, so please update that as well with a dedicated PR.

[amwill04]

There is a massive gotcha in this method though. All errors will propogate into the onError method. So if I throw and error within my main handler it isn't handled as I would expect. This may be fine if you have one async middleware. But what happens if you have more than one? Each onError then runs in order? What if on certain errors you want to exit early but on others you don't?

Im struggling to succintly run async before method exit early on errors but still handle errors differently in main handler.

I assume this soloution is to have a middleware that simple is registered first to handle all and every error? The above solution the onError wont run if I have other middleware with onError registered and dont have next() and/or middleware registered after wont run because the above is hitting the escape hatch.

Sorry for the million questions. I have taken over something from someone else and think I am completely misusing middy

[amwill04]

For my unasked for and maybe unwarrented 10c worth it would seem more obviouse if onError that is regestered in a middleware object is only ran when that middleware errors and there be a main onError.

[lmammino]

Happy about all the questions @amwill04 and, in all honesty, you bring up a very interesting conversation. Developer ergonomics are pretty important in every framework so I am happy for those to be challenged.

I'll try to give you my rationale based on the current implementation and feel free to disagree if it doesn't seem intuitive or correct to you.

Quoting the docs:

When there is an error, the regular control flow is stopped and the execution is moved back to all the middlewares that implement a special phase called onError, following the order they have been attached.

The idea here is that there could be special middlewares that only deal with managing errors (for a built-in example you can check out the http-error-handler middleware).

This specific middleware is convenient when doing HTTP APIs through lambdas. In such cases, I believe, that in your main handler, you shouldn't be too much worried about formatting the response correctly in case of error, but you should simply throw that error in a way that an error handler will be able to catch and provide a correct HTTP error response to the lambda runtime.

This middleware will check if the error actually looks like an HTTP error. If it does then it decides to terminate the flow by providing a valid HTTP response. If it doesn't, it just calls next() to allow other error middlewares to do something with it.

I reckon this approach is maybe not the most intuitive but I feel it gives a lot of flexibility.

amwill04 commented 21 hours ago: For my unasked for and maybe unwarrented 10c worth it would seem more obvious if onError that is registered in a middleware object is only ran when that middleware errors and there be a main onError.

Also, to answer this last point you made, I believe that if you want to catch errors only in your given middleware you could do that with an internal try-catch, you shouldn't probably need the framework to do anything special for you.

A final comment I would like to make is that I really get that trying to support async-await, promise-based and callback-based middlewares and handlers are tricky and it makes dealing with all these edge cases quite confusing. I admit I as well struggle to remember what's the right way to do something sometimes.

Considering that async-await is becoming sort of a standard and Node.js 8 is now the new bare minimum (and shortly node 10 will take that spot!), one of the things I am considering for future major versions of middy is to totally drop support for callbacks and only rely on Promises and Async Await. This is not a hard decision yet, but I feel it will help to simplify the codebase and make the framework easier to understand and hence more accessible to new adopters.

I hope all of this makes sense and helps to provide more context around the current decisions.

[amwill04]

Hey @lmammino thanks for the response. Reading the http-error-handler was the lightbulb moment I think I was needing. I was basically trying to implement that anyway.

Im going to put together an example with all this together and hopefully help someone else, if thats cool?

[lmammino]

Thanks again to you, @amwill04! I would absolutely love any contribution, especially if it can help other people to ramp up quickly! :)

[shinglyu]

Hi we hit the exact same issue. We have an async before function, and we accidentally called return next(). This causes the main handler to be called twice. Something along the line:

const asyncValidator = () => {
  before: async (handler) => {
    await asyncValidate(handler.event.body)
    // some other suff
    return next() // This cause the handler to run twice
  }
}

Besides making this more clear ("You should only return, but not return next()), can we somehow add a runtime check so people will get an error when using return next()?

Also an example for early exiting an async before handler would be very valuable.
@amwill04 are you working on any PR to update the documentation? If not, should I be updating the README directly or are there other docs?

[amwill04]

Hey @shinglyu if I’m completely honest I’ve ripped out Middyjs from our codebase, it was overkill for our us - nothing to do with middyjs. So, I’ve completely forgotten what I was doing here. It’s worth looking at the source code in http error package I referenced above. Really helped me.