Request to allow return of an object or string instead of response, assuming 200

[zoe-edwards]

With Node 8.10 and #24 merged in to 0.4.0, we can now return promises 👌 (thanks @leonardodino)

However, it doesn’t behave the same way Lambda does.

Using 8.10, you can return a promise to Lambda. It reads the resolve as being a 200 (with the body), and a reject as a 500 (with the error).

However Netlify Lambda this doesn’t work yet:

Response with status NaN in 0 ms.
(node:6095) UnhandledPromiseRejectionWarning: RangeError [ERR_HTTP_INVALID_STATUS_CODE]: Invalid status code: undefined
    at ServerResponse.writeHead (_http_server.js:201:11)
    at ServerResponse.writeHead (/Users/thomas/Sites/netlify-lambda-promise-test/node_modules/on-headers/index.js:55:19)
    at ServerResponse._implicitHeader (_http_server.js:192:8)
    at write_ (_http_outgoing.js:637:9)
    at ServerResponse.write (_http_outgoing.js:622:10)
    at callback (/Users/thomas/Sites/netlify-lambda-promise-test/node_modules/netlify-lambda/lib/serve.js:26:14)
    at /Users/thomas/Sites/netlify-lambda-promise-test/node_modules/netlify-lambda/lib/serve.js:43:7
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:160:7)
(node:6095) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 2)

This is using the test project I created to test with.

This should work, but just times out with the above error:

exports.handler = async event => {
    return new Promise((resolve, reject) => {
        resolve("yay!");
    });
};

This does work:

exports.handler = async event => {
    return new Promise((resolve, reject) => {
        resolve({ statusCode: 200, body: "yay!" });
    });
};

So promises are being resolved, but it's not allowing it to return a promise. Now I’m new to promises, so this suggestion might be wrong or there might be a better way, but if we change this:

promise.then(
  function(data) {
    callback(null, data);
  },
  function(err) {
    callback(err, null);
  }
);

to this:

promise.then(
  function(data) {
    callback(null, { statusCode: 200, body: data });
  },
  function(err) {
    callback(err, null);
  }
);

Then it will work – but I’m not sure how safe this is or if it’s a good idea or not.

[zoe-edwards]

Another idea might be to re-write the callback function to something like this:

function createCallback(response) {
  return function callback(err, lambdaResponse) {
    if (err) {
      return handleErr(err, response);
    }

    if (typeof lambdaResponse === 'string') {
      lambdaResponse = {
        body: lambdaResponse,
        statusCode: 200
      }
    }

    response.statusCode = lambdaResponse.statusCode;
    for (const key in lambdaResponse.headers) {
      response.setHeader(key, lambdaResponse.headers[key]);
    }
    response.write(
      lambdaResponse.isBase64Encoded
        ? base64.decode(lambdaResponse.body)
        : lambdaResponse.body
    );
    response.end();
  };
}

[leonardodino]

nice catch!

indeed, I didn't read the whole AWS document to implement that, and I'm personally returning {statusCode, headers, body} on my functions, but we should definitely follow the specs (:

[zoe-edwards]

I’ve been reading up on this to try and figure out how to put together a PR suggestion. However, it is remarkably complex, even though it’s only replacing 1 line of code. I’m going to note it down here incase somebody in the future stumbles into this area.

The ‘Netlify Functions’ product is actually ‘Netlify Lambda + API Gateway something like API Gateway (Edit) + proxy’, but I think I can see why they went with the snappier function.

Lambda functions, when left to their own devices, will return a 200 and whatever you return if successful. If they fail, they throw a 500. Strictly speaking, there is no way to actually control the response status code just using Lambda.

However, Netlify have paired it up with (what looks like) API Gateway behind the scenes – or have rolled their own that mimics its behaviour somewhat.

When you return a response object, complete with body and headers, that’s designed for API Gateway’s api_proxy, which is very different to a Lambda function. So when you include statusCode in your response, you’re not actually setting the status of the Lambda request, but you’re telling API Gateway to use this. (Technically I believe the Lambda response is still 200, even though you’re written statusCode: 201 or something.)

My original request was that the discovery of allowing Lambda functions to return just a promise is actually not relatable to the way Netlify Functions works. If you’re using Lambda without API Gateway proxying the request, you’ve always been able to return just a string, and it would be returned as a 200. There is no way to override the status code just using Lambda, without using API Gateway as a proxy. So the fact that it’s able to resolve a promise and return the value is irrelevant.

So, having dug deep into this now, I have realised that my request is not a bug, but actually a feature request.

My dream is that I can have a chain of functions that pass a promise all the way back up to the handler, which when resolved is the value of body.

I have structured my functions using Middy as a ‘middleware’, and then organised everything into an MVC-style pattern. It’s not actually using a framework, but the structure mimics MVC. In this pattern, when a DynamoDB promise returns the contents of a file, I don’t want to declare the status code of the response at this point. These functions are shared, so it’s not the responsibility of a model function to set an HTTP status.

My request is that I’d like to be able to return an object as the resolve of a promise, and that Netlify Lambda would assume that object is good to send on to the user.

However, to mimic the behaviour of Lambda, this should also be valid if it’s not an object (a string), and it should also behave that way if it is returned outside of a promise.

I think that in the most TL;DR way:

Would it be okay to allow the return of an object, that’s converted into a JSON string and the content type set, or a string, and then allow it to return that with a 200?

If you’d like to override it, or set headers, then you can just return that.

I guess the test would be – if an object is returned and it has both a statusCode and body, then it’s good to return as-is. But if they are both missing, create a response and return that.

If I had more time I would write a shorter comment.

[swyxio]

no stalebot, this is a really good issue!

@ThomasEdwards and @8eecf0d2 you folks have thought about this far more than i have. would happily take a PR for the intuitive behavior.

[8eecf0d2]

Just tested out thomasedwards/netlify-lambda-promise-test at gifted-kalam-e7b693.netlify.com.

• async.js returns 500
• promised_land.js returns 500
• old_school.js returns 200

I think this would be a good feature to support but not until Netlify itself supports it, which I think @ThomasEdwards is alluding too when he suggests this is a feature request (for Netlify Functions itself, not netlify-lambda) and not a bug.

@sw-yx anywhere in particular we should +1 this as a feature request for Netlify Functions?

[swyxio]

theres no open voting system here but your suggestion is valued and noted. fwiw we actually dont use API Gateway for Netlify Functions. its probably something else but i dont know enough to tell you what it is. will shoot this over to the product/support folk as a feature request. i'm a little worried that it may break behavior folks currently rely on.

[zoe-edwards]

My hope was that if a user is still using callbacks, then no change would be required. As currently you cannot return a Promise, nobody using Netlify Functions should be using this behaviour, so my hope is that if it were supported, it would not cause any backwards compatibility issues. However, I do not know the inner working of Netlify Functions, so I understand this might have consequences!

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[swyxio]

boop

[zoe-edwards]

boop boop