feat: typing ingest lambda and related files, app entry, adjusting types as needed#1087
Conversation
| }, | ||
| "editor.tabSize": 2, | ||
| "files.insertFinalNewline": true, | ||
| "typescript.tsserver.maxTsServerMemory": 4096, |
There was a problem hiding this comment.
temporary as it needs more memory in the mixed memory codebase, will remove on final PR to reduce this allocation once it's all typescript
pjhartzell
left a comment
pjhartzell
left a comment
There was a problem hiding this comment.
I left a few comments, the most important of which relates to the API app being re-created on each Lambda handler invocation.
| const isSqsEvent = (event: SQSEvent | ApiRecord): event is SQSEvent => 'Records' in event | ||
|
|
||
| const isSnsMessage = (record) => record.Type === 'Notification' | ||
| const isSnsMessage = (record): record is SNSMessage => record.Type === 'Notification' |
There was a problem hiding this comment.
Should we type record here in a manner similar to how it is done in isStacRecordRef above?
const isSnsMessage = (record: unknown): record is SNSMessage =>
typeof record === 'object'
&& record !== null
&& 'Type' in record
&& record.Type === 'Notification'
| event: APIGatewayProxyEvent, | ||
| context: Context | ||
| ): Promise<APIGatewayProxyResult> => { | ||
| const appInstance = await createApp() |
There was a problem hiding this comment.
I don't think we want to move app creation from module scope (where it gets created once and reused across every warm Lambda invocation) to inside of callServerlessApp here, because callServerlessApp is called inside the handler function, which means the app gets created on every invocation (the handler function is run on every invocation) for a warm Lambda. I'm guessing that warm Lambda invocations are more the rule than the exception for the api. This is likely a non-trivial performance hit at scale.
The ingest lambda uses a pattern in index.ts that caches at the module level on the first call (see assetProxy and getAssetProxy) that could be a solution. Or not.
| getObjectBody(s3Location).then((b) => b.transformToString()) | ||
|
|
||
| const getObjectJson = (s3Location: {bucket: string, key: string}): Promise<unknown> => | ||
| const getObjectJson = (s3Location: {bucket: string, key: string}): Promise<StacItem> => |
There was a problem hiding this comment.
Without validation, I don't think we should use StacItem in the return. The function is a rather generic parsing of text to json. I think we should keep it as unknown here, and cast (or validate) to what we think it should be in the calling function. So this function just returns what its name (getObjectJson) indicates—some unknown json.
This function is currently called in only one place: src/lambdas/ingest/index.ts. Perhaps just add as StacRecord to the call site?
| const parseEvent = ( | ||
| rawEvent: APIGatewayProxyEvent | ||
| ): APIGatewayProxyEvent => { |
There was a problem hiding this comment.
I think the purpose of parseEvent is to validate that some untrusted (unknown) input and validate that it is an APIGatewayProxyEvent. So the input rawEvent should not be typed as an APIGatewayProxyEvent—only the output should be. The follow-on from this is that the call to parseEvent in the handler should likely not be passing something already typed as APIGatewayProxyEvent (which the handler event arg is typed as).
2 participants
Related Issue(s):
Typing the lambda entry points, index files, and continuing to refine types as necessary for the typescript migration.
Proposed Changes:
PR Checklist: