Add DynamoDB APL to Segment app #1690
Conversation
🦋 Changeset detectedLatest commit: af4d5f6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
6 Skipped Deployments
|
krzysztofzuraw
added
the
skip changeset
apps/segment/src/lib/dynamodb-apl.ts
Outdated
| static MissingEnvVariablesError = BaseError.subclass("MissingEnvVariablesError"); | ||
|
|
||
| constructor({ segmentAPLEntity }: { segmentAPLEntity: SegmentAPLEntityType }) { | ||
| this.segmentAPLRepository = new SegmentAPLRepository({ segmentAPLEntity }); |
apps/segment/src/lib/dynamodb-apl.ts
Outdated
| }); | ||
|
|
||
| if (getEntryResult.isErr()) { | ||
| // TODO: should we throw here? |
There was a problem hiding this comment.
no, because APL interface is allowing undefined as a non-existing value
but, we should check:
- if its 404-like error, map to underfined
- if its connection error/db down etc, we should throw (panic mode)
There was a problem hiding this comment.
I'm wondering if we have a easy way of knowing if there is connection error - I checked and toolbox doesn't expose that info if it throws error
There was a problem hiding this comment.
I think:
- Every promise rejection from dynamo is "unhandled"
- Resolved promise with
Item === nullis correct request, but empty database row
There was a problem hiding this comment.
Makes sense - I will update implementation to add such logic
apps/segment/src/lib/dynamodb-apl.ts
Outdated
| import { SegmentAPLEntityType } from "@/modules/db/segment-main-table"; | ||
|
|
||
| export class DynamoAPL implements APL { | ||
| private segmentAPLRepository: SegmentAPLRepository; |
There was a problem hiding this comment.
nitpick, but the class is APL and app context is Segment, so you "know" the outer context - you dont have to prefix it. Calling variable "repo" or "repository" is verbose enough
apps/segment/src/lib/dynamodb-apl.ts
Outdated
| const getAllEntriesResult = await this.segmentAPLRepository.getAllEntries(); | ||
|
|
||
| if (getAllEntriesResult.isErr()) { | ||
| // TODO: should we throw here? |
There was a problem hiding this comment.
same as above- depending on the error type
| if (!getEntryResult.value.Item) { | ||
| this.logger.warn("APL entry not found", { args }); | ||
|
|
||
| return err(new SegmentAPLRepository.ReadEntityError("APL entry not found")); | ||
| } |
There was a problem hiding this comment.
you can also consider less verobse interface: ResultAsync<AuthData | null, Error> where we push null as a successful data flow, separated from error flow. I'm not highly opinionated here, but handling will be less verbose than instanceOf
| return err(setEntryResult.error); | ||
| } | ||
|
|
||
| return ok(undefined); |
There was a problem hiding this comment.
do we have anything valuable returned from dynamo?
There was a problem hiding this comment.
I checked and we have:
ConsumedCapacityToolboxItem- entry itself
I'm wondering if we need this data in APL class?
There was a problem hiding this comment.
I think this happens so rarely it doesn't matter, but preferably if we don't return anything, we can save transfer from Dynamo too (don't fetch what we don't need).
| typeof Table< | ||
| { name: "PK"; type: "string" }, | ||
| { | ||
| name: "SK"; | ||
| type: "string"; |
There was a problem hiding this comment.
can you put these genetic to a variable? its cryptic and hard to read
apps/segment/src/saleor-app.ts
Outdated
| case "dynamodb": { | ||
| if ( | ||
| !env.DYNAMODB_MAIN_TABLE_NAME || | ||
| !env.AWS_REGION || | ||
| !env.AWS_ACCESS_KEY_ID || | ||
| !env.AWS_SECRET_ACCESS_KEY | ||
| ) { | ||
| throw new MisconfiguredDynamoAPLError( | ||
| "DynamoDB APL is not configured - missing env variables. Check saleor-app.ts", | ||
| ); | ||
| } |
Differences Found✅ No packages or licenses were added. SummaryExpand
|
krzysztofzuraw
removed
the
skip changeset
vercel
bot
temporarily deployed
to
Preview – saleor-app-products-feed
Scope of the PR
Added DynamoDB APL. This APL is using DynamoDB as storage.
Related issues
Checklist