issue #9: ISailplaneAPIGatewayProxyEvent Interface #8
Conversation
voodooGQ
changed the title
|
I think the package-lock changes should be dropped from this (since no dependencies are being changed). |
|
@troyready Sounds good, I'll push up that adjustment |
lambda-utils/lib/lambda-utils.ts
Outdated
| * Casted interface for APIGatewayProxyEvents converted through the middleware | ||
| * | ||
| * @typedef {Interface} | ||
| * @property {any} body The cast JSON -> object body element |
There was a problem hiding this comment.
We don't need these tags - they are useless. Typescript removes the need for some JsDoc that violates the DRY principle, and body is being document by its own JsDoc.
lambda-utils/lib/lambda-utils.ts
Outdated
| /** | ||
| * Cast JSON -> object body element | ||
| * @property body | ||
| * @type {any} |
There was a problem hiding this comment.
Remove repetitive JsDoc.
Let's actually describe what this property is: HTTP Request body, parsed from a JSON string into an object.
lambda-utils/lib/lambda-utils.ts
Outdated
| * @typedef {Interface} | ||
| * @property {any} body The cast JSON -> object body element | ||
| */ | ||
| export interface ISailplaneAPIGatewayProxyEvent extends APIGatewayProxyEvent { |
There was a problem hiding this comment.
Unless you work at Microsoft, let's not use Hungarian notation. 😜
I'm tempted to call this APIGatewayProxyEvent still. Since LambdaUtils is used as import * as LambdaUtils from "@sailplane/lambda-utils", it would become LambdaUtils.APIGatewayProxyEvent to distinguish it.
In order to do this declaration, you add as AWS_APIGatewayProxyEvent to the import, and then use that name here.
There was a problem hiding this comment.
One of the linter rules I have states to keep Interface prefixed with an "I". Is that not the case in normal practice? I'm hoping not because I'd love to turn that rule off
There was a problem hiding this comment.
I've never seen that rule. Turn it off.
palantir/tslint#1520
There was a problem hiding this comment.
^ That's the rule I'm referring to :)
| * @type {any} | ||
| */ | ||
| body: any; | ||
| } |
There was a problem hiding this comment.
Since LambdaUtils also de-nulls queryStringParameters and pathParameters, can do those too:
pathParameters: { [name: string]: string };
queryStringParameters: { [name: string]: string };
lambda-utils/lib/lambda-utils.ts
Outdated
| * HTTP Request body, parsed from a JSON string into an object. | ||
| * @property body | ||
| */ | ||
| body: any; |
There was a problem hiding this comment.
In speaking with a colleague, I remember that this transformation only happens when the received body has JSON content. So the body can be left untouched, as string | null. Since any includes string, please just add the | null which will be true if no body was received.
lambda-utils/lib/lambda-utils.ts
Outdated
|
|
||
| /** | ||
| * HTTP URL query string parameters, parsed from a JSON string into an object | ||
| * @property pathParameters |
There was a problem hiding this comment.
Please explain to me the value of this tag. Just seems like an opportunity to be out of sync with the actual code. (Which it is indeed right here.)
There was a problem hiding this comment.
I'm not sure what you want me to explain on this one. It's a standard jsdoc element. If you'd prefer it removed I'll happily do so.
There was a problem hiding this comment.
There was a problem hiding this comment.
From that page "This tag is intended for simple collections of static properties". See the example. This is not what that tag is for.
There was a problem hiding this comment.
Noted, and already addressed
lambda-utils/lib/lambda-utils.ts
Outdated
| pathParameters: { [name: string]: string }; | ||
|
|
||
| /** | ||
| * HTTP URL query string parameters, parsed from a JSON string into an object |
There was a problem hiding this comment.
Sorry... just a pair of mistakes here in the descriptions of both of these two new properties. They are not parsed from JSON strings. What LambdaUtils does is guarantee that they are defined. So they will be an empty object rather than null if there are no path parameters or query string. Maybe the part after the comma on both of these would read "always defined, never null."
There was a problem hiding this comment.
Sorry just seeing this, I'll go in and make that change quick.
Addresses a simple issue when utilizing AWS.APIGatewayProxyEvent items where the
bodyattribute is considered incorrect bytsconfig. The AWS.APIGatewayProxyEvent has this cast astring, however the middleware converts it to an object.Still getting up to speed on Typescript, so if there's a better way to handle this I'm all ears.
Very large package-lock change. Don't know if we want to do that as a separate PR or not.