feat: init db wrapper #7
Conversation
|
This PR will trigger a minor release when merged. |
| const body = { | ||
| message, | ||
| timestamp: new Date().toISOString(), | ||
| }; | ||
|
|
||
| // Set up the parameters for the send message command | ||
| const params = { | ||
| DelaySeconds: 10, |
| import queueWrapper from '../src/queue-wrapper.js'; | ||
| import SQSQueue from '../src/sqs-queue.js'; | ||
|
|
||
| describe('Queue Wrapper Tests', () => { |
|
@alinarublea please also add tests for the other newly added parts |
src/db-wrapper.js
Outdated
| break; | ||
| } | ||
| }; | ||
| 'use strict'; |
There was a problem hiding this comment.
not needed. ESM is always strict.
| 'use strict'; |
| if (!context.db) { | ||
| context.db = new DB(context); | ||
| } |
There was a problem hiding this comment.
- what is the benefit of having this in a wrapper?
- as mentioned earlier, do not cache directly in
context, but incontext.attributes - do you need to cleanup/close/discard the
DBafter use?
There was a problem hiding this comment.
@tripodsan we were following the pattern from storage which is included directly in the context https://github.com/adobe/helix-universal/blob/7a63f8ddc179d5f91c1c170b40bf4bad4a17e769/src/aws-adapter.js#L158, and it s not an attribute. Sure we will cleanup after use.
src/db.js
Outdated
| return newAudit; | ||
| await this.saveRecord(newAudit, TABLE_AUDITS); | ||
| this.log.info(`Audit for domain ${site.domain} saved successfully`); | ||
| return Promise.resolve(newAudit); |
There was a problem hiding this comment.
the return type of a async function is automatically a promise.
| return Promise.resolve(newAudit); | |
| return newAudit; |
src/db.js
Outdated
| * Saves an audit to the DynamoDB. | ||
| * @param {object} site - Site object containing details of the audited site. | ||
| * @param {object} audit - Audit object containing the type and result of the audit. | ||
| * @returns {Promise<void>} Resolves once audit is saved. |
There was a problem hiding this comment.
not true.... returns the new adit object
| const now = new Date().toISOString(); | ||
| const uuid = Date.now().toString(); | ||
| const uuid = Date.now() | ||
| .toString(); | ||
|
|
||
| const newAudit = { |
There was a problem hiding this comment.
maybe it would help to include a typedef for audit and site
either as jsdoc comment @typedef Audit ... or as typescript type
src/index.js
Outdated
| if (!message.domain) { | ||
| return new Response('', { | ||
| status: 400, | ||
| headers: { | ||
| 'x-error': 'Event message does not contain a domain', | ||
| }, | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
move this up. cheap checks should come first new object allocation.
src/psi-client.js
Outdated
| @@ -10,7 +10,6 @@ | |||
| * governing permissions and limitations under the License. | |||
| */ | |||
| import axios from 'axios'; | |||
| @@ -136,6 +135,7 @@ function PSIClient(config) { | |||
| * @returns {Promise<object>} The processed PageSpeed Insights audit data. | |||
| */ | |||
| const performPSICheck = async (domain, strategy) => { | |||
| // eslint-disable-next-line no-useless-catch | |||
There was a problem hiding this comment.
why not follow the eslint advice here ? :-)
| if (!context.queue) { | ||
| context.queue = new SqsQueue(context); | ||
| } |
| context.queue = new SqsQueue(context); | ||
| } | ||
|
|
||
| return func(request, context); |
There was a problem hiding this comment.
probably close/discard queue client ?
| @@ -22,11 +21,11 @@ function storage(config) { | |||
| ContentType: 'application/json', | |||
| }; | |||
|
|
|||
| // eslint-disable-next-line no-useless-catch | |||
Co-authored-by: Tobias Bocanegra <[email protected]>
Please ensure your pull request adheres to the following guidelines:
Related Issues
Thanks for contributing!