feat: create wrapper for SQSClient #9
Conversation
|
This PR will trigger a minor release when merged. |
src/index.js
Outdated
| return new Response('SUCCESS'); | ||
| } | ||
|
|
||
| export const main = wrap(run) | ||
| .with(helixStatus) | ||
| .with(logger.trace) | ||
| .with(logger) | ||
| .with(secrets); | ||
| .with(secrets) | ||
| .with(queueWrapper); |
There was a problem hiding this comment.
secrets wrapper should be in outer level then queueWrapper otherwise queueWrapper won't have access to the env variables
src/queue-wrapper.js
Outdated
| throw new Error('AUDIT_RESULTS_QUEUE_URL env variable is empty/not provided'); | ||
| } | ||
|
|
||
| context.queue = SqsQueue(region, queueUrl, log); |
There was a problem hiding this comment.
nitpicking: I would check if context.queue already defined here, and remove the if check in the SQSQueues constructor
| context.queue = SqsQueue(region, queueUrl, log); | |
| if (!context.queue) { | |
| context.queue = SqsQueue(region, queueUrl, log); | |
| } |
see the suggestion below as well
There was a problem hiding this comment.
please note: that we store all extra (cached) objects in context.attributes and not in context directly. maybe you can follow this convention as well.
- https://github.com/adobe/helix-universal/blob/7a63f8ddc179d5f91c1c170b40bf4bad4a17e769/src/aws-adapter.js#L160
- https://github.com/adobe/helix-admin-support/blob/7f723a66333c6fe0085a4988ed2db65a98ed6375/src/index.d.ts#L149
eg: - https://github.com/adobe/helix-admin-support/blob/7f723a66333c6fe0085a4988ed2db65a98ed6375/src/contentbus.js#L67-L69
There was a problem hiding this comment.
@tripodsan thank you! @dzehnder let's stick to the same convention
There was a problem hiding this comment.
@tripodsan could you give me access to helix-admin-support repo I don t seem to have it
There was a problem hiding this comment.
We took a look and we will add the queue and storage to the context, but I think we shouldn t add it to the attributes, because I see the storage which is similar to db and queue is added directly to the context. Wdyt @tripodsan ?
src/sqs-queue.js
Outdated
| export default function SQSQueue(region, queueUrl, log) { | ||
| if (!sqsClient) { | ||
| sqsClient = new SQSClient({ region }); | ||
| log.info(`Creating SQS client in region ${region}`); | ||
| } |
There was a problem hiding this comment.
continues from the suggestion above
| export default function SQSQueue(region, queueUrl, log) { | |
| if (!sqsClient) { | |
| sqsClient = new SQSClient({ region }); | |
| log.info(`Creating SQS client in region ${region}`); | |
| } | |
| export default function SQSQueue(region, queueUrl, log) { | |
| const sqsClient = new SQSClient({ region }); | |
| log.info(`Creating SQS client in region ${region}`); | |
|
Moved this PR content to #7 as it is depending on the changes from there |
Please ensure your pull request adheres to the following guidelines:
Related Issues
Thanks for contributing!