Strict check of function value on middleware return value

[dbenavidesv]

Hey. Why not a strict check for a function type here? It seems like it would add a bit more security and not left this relying partially on types. Is there a reason for not checking explicitly?

https://github.com/apollo-server-integrations/apollo-server-integration-aws-lambda/blob/a5f2461912c16eeb511d704affd56f1a1374b9bb/src/lambdaHandler.ts#L87

[dbenavidesv]

Just asking :D

[BlenderDude]

We definitely could, but I'll state the case for not!

I personally like the fact that Typescript allows me to shed some of the runtime type-checking work to compile-time. If we implement a runtime check here, what might happen if a rogue string was returned from the middleware instead of an object or function? Well, realistically we'd want the function to throw, and maybe state something like "Middleware return value must be either a Function or Result object". This would require a few extra lines of code and add complexity to the runtime that I think is unnecessary. Typescript would give a similar (more detailed) error during compile time if you tried to do something funky like return a string, meaning a runtime check would be redundant and waste precious cpu cycles!

Furthermore, if I add a check for a function type, that still doesn't guarantee it's the right function. What if someone just returns () => "Haha gotcha!"? That would pass a static "is it a function" check, but when called it would likely throw when lambda tries to parse "Haha gotcha!" as a result object. So to do a full check, we'd need to parse the output of the middleware function as well. Again, adding more runtime code that could be determined at compile time. Doing this throughout the library at every junction between user code and library code would likely make this library 10x larger than it currently stands.

Given that, I think the onus is on the developer using the library to respect the type definitions. At that line of code, Typescript tells me it is either a LambdaResponse<RequestHandlerResult<RH>> or undefined. With that in mind, a positivity check if (middlewareReturnValue) tells me everything I need to know! If it's a function, it will resolve to true, and everything inside the if can be assured it is in the presence of LambdaResponse<RequestHandlerResult<RH> as undefined would be falsy and not enter the if block.

Hope this gives some insight into my thought process 😄 Feel free to follow up!

[dbenavidesv]

Yeah it makes sense, I thought that maybe someone can circumvent the type checks and send something funky but if they do that, it's not a library issue. Thanks for the response.