-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
remove built in webhook response builder #413
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
--- | ||
"@saleor/app-sdk": major | ||
--- | ||
|
||
Removed `ctx.buildResponse` parameter from SyncWebhookHandler ctx and replace with standalone `buildSyncWebhookResponsePayload` function | ||
|
||
Before | ||
|
||
```typescript | ||
|
||
new SaleorSyncWebhook(...).createHandler( | ||
req, res, ctx | ||
) { | ||
|
||
const typeSafePayload = ctx.buildResponse({ | ||
// this must be valid response | ||
}) | ||
} | ||
``` | ||
|
||
After | ||
|
||
```typescript | ||
import { buildSyncWebhookResponsePayload } from "@saleor/app-sdk/handlers/shared"; | ||
|
||
new SaleorSyncWebhook(...).createHandler( | ||
req, res, ctx | ||
) | ||
{ | ||
|
||
const typeSafePayload = buildSyncWebhookResponsePayload<"ORDER_CALCULATE_TAXES">({ | ||
// this must be valid shape | ||
}) | ||
} | ||
``` | ||
|
||
This change reduces complexity of TypeScript generics and make it easier to build abstractions on top of built-in handlers |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
import { afterEach, describe, expect, it, vi } from "vitest"; | ||
|
||
import { FormatWebhookErrorResult } from "@/handlers/shared"; | ||
import { buildSyncWebhookResponsePayload, FormatWebhookErrorResult } from "@/handlers/shared"; | ||
import { SaleorWebhookValidator } from "@/handlers/shared/saleor-webhook-validator"; | ||
import { MockAPL } from "@/test-utils/mock-apl"; | ||
|
||
|
@@ -27,20 +27,20 @@ describe("AWS Lambda SaleorSyncWebhook", () => { | |
it("returns valid URL when baseUrl has Lambda stage", () => { | ||
const webhook = new SaleorSyncWebhook(webhookConfig); | ||
expect(webhook.getWebhookManifest("https://aws-lambda.com/prod").targetUrl).toBe( | ||
"https://aws-lambda.com/prod/api/webhooks/checkout-calculate-taxes" | ||
"https://aws-lambda.com/prod/api/webhooks/checkout-calculate-taxes", | ||
); | ||
expect(webhook.getWebhookManifest("https://aws-lambda.com/prod/").targetUrl).toBe( | ||
"https://aws-lambda.com/prod/api/webhooks/checkout-calculate-taxes" | ||
"https://aws-lambda.com/prod/api/webhooks/checkout-calculate-taxes", | ||
); | ||
expect(webhook.getWebhookManifest("https://aws-lambda.com/test").targetUrl).toBe( | ||
"https://aws-lambda.com/test/api/webhooks/checkout-calculate-taxes" | ||
"https://aws-lambda.com/test/api/webhooks/checkout-calculate-taxes", | ||
); | ||
}); | ||
|
||
it("returns valid URL when baseURl doesn't have lambda stage ($default stage)", () => { | ||
const webhook = new SaleorSyncWebhook(webhookConfig); | ||
expect(webhook.getWebhookManifest("https://aws-lambda.com/").targetUrl).toBe( | ||
"https://aws-lambda.com/api/webhooks/checkout-calculate-taxes" | ||
"https://aws-lambda.com/api/webhooks/checkout-calculate-taxes", | ||
); | ||
}); | ||
|
||
|
@@ -50,16 +50,16 @@ describe("AWS Lambda SaleorSyncWebhook", () => { | |
webhookPath: `/${webhookConfig.webhookPath}`, | ||
}); | ||
expect(webhook.getWebhookManifest("https://aws-lambda.com/prod").targetUrl).toBe( | ||
"https://aws-lambda.com/prod/api/webhooks/checkout-calculate-taxes" | ||
"https://aws-lambda.com/prod/api/webhooks/checkout-calculate-taxes", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: maybe in the other PR we can run prettier through codebase to fix formatting issues like that? 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, dont know why it didnt work on pre-commit on previous prs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I do know - I updated prettier and it's defaults changed |
||
); | ||
expect(webhook.getWebhookManifest("https://aws-lambda.com/prod/").targetUrl).toBe( | ||
"https://aws-lambda.com/prod/api/webhooks/checkout-calculate-taxes" | ||
"https://aws-lambda.com/prod/api/webhooks/checkout-calculate-taxes", | ||
); | ||
expect(webhook.getWebhookManifest("https://aws-lambda.com/test").targetUrl).toBe( | ||
"https://aws-lambda.com/test/api/webhooks/checkout-calculate-taxes" | ||
"https://aws-lambda.com/test/api/webhooks/checkout-calculate-taxes", | ||
); | ||
expect(webhook.getWebhookManifest("https://aws-lambda.com/").targetUrl).toBe( | ||
"https://aws-lambda.com/api/webhooks/checkout-calculate-taxes" | ||
"https://aws-lambda.com/api/webhooks/checkout-calculate-taxes", | ||
); | ||
}); | ||
}); | ||
|
@@ -88,15 +88,15 @@ describe("AWS Lambda SaleorSyncWebhook", () => { | |
|
||
const handler: AwsLambdaSyncWebhookHandler<Payload> = vi | ||
.fn() | ||
.mockImplementation(async (_event, _context, ctx) => ({ | ||
.mockImplementation(async () => ({ | ||
statusCode: 200, | ||
body: JSON.stringify( | ||
ctx.buildResponse({ | ||
buildSyncWebhookResponsePayload<"CHECKOUT_CALCULATE_TAXES">({ | ||
lines: [{ tax_rate: 8, total_net_amount: 10, total_gross_amount: 10.8 }], | ||
shipping_price_gross_amount: 2.16, | ||
shipping_tax_rate: 8, | ||
shipping_price_net_amount: 2, | ||
}) | ||
}), | ||
), | ||
})); | ||
const wrappedHandler = saleorSyncWebhook.createHandler(handler); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,34 +1,28 @@ | ||
import { SyncWebhookInjectedContext } from "@/handlers/shared"; | ||
import { buildSyncWebhookResponsePayload } from "@/handlers/shared/sync-webhook-response-builder"; | ||
import { SyncWebhookEventType } from "@/types"; | ||
|
||
import { AWSLambdaHandler } from "../platform-adapter"; | ||
import { AwsLambdaWebhookHandler, SaleorWebApiWebhook, WebhookConfig } from "./saleor-webhook"; | ||
|
||
export type AwsLambdaSyncWebhookHandler< | ||
TPayload, | ||
TEvent extends SyncWebhookEventType = SyncWebhookEventType, | ||
> = AwsLambdaWebhookHandler<TPayload, SyncWebhookInjectedContext<TEvent>>; | ||
> = AwsLambdaWebhookHandler<TPayload>; | ||
|
||
export class SaleorSyncWebhook< | ||
TPayload = unknown, | ||
TEvent extends SyncWebhookEventType = SyncWebhookEventType, | ||
> extends SaleorWebApiWebhook<TPayload, SyncWebhookInjectedContext<TEvent>> { | ||
> extends SaleorWebApiWebhook<TPayload> { | ||
readonly event: TEvent; | ||
|
||
protected readonly eventType = "sync" as const; | ||
|
||
protected extraContext = { | ||
buildResponse: buildSyncWebhookResponsePayload<TEvent>, | ||
}; | ||
|
||
constructor(configuration: WebhookConfig<TEvent>) { | ||
super(configuration); | ||
|
||
this.event = configuration.event; | ||
} | ||
|
||
createHandler(handlerFn: AwsLambdaSyncWebhookHandler<TPayload, TEvent>): AWSLambdaHandler { | ||
createHandler(handlerFn: AwsLambdaSyncWebhookHandler<TPayload>): AWSLambdaHandler { | ||
return super.createHandler(handlerFn); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,34 +1,25 @@ | ||
import { SyncWebhookInjectedContext } from "@/handlers/shared"; | ||
import { buildSyncWebhookResponsePayload } from "@/handlers/shared/sync-webhook-response-builder"; | ||
import { SyncWebhookEventType } from "@/types"; | ||
|
||
import { WebApiHandler } from "../platform-adapter"; | ||
import { SaleorWebApiWebhook, WebApiWebhookHandler, WebhookConfig } from "./saleor-webhook"; | ||
|
||
export type WebApiSyncWebhookHandler< | ||
TPayload, | ||
TEvent extends SyncWebhookEventType = SyncWebhookEventType | ||
> = WebApiWebhookHandler<TPayload, SyncWebhookInjectedContext<TEvent>>; | ||
export type WebApiSyncWebhookHandler<TPayload> = WebApiWebhookHandler<TPayload>; | ||
|
||
export class SaleorSyncWebhook< | ||
TPayload = unknown, | ||
TEvent extends SyncWebhookEventType = SyncWebhookEventType | ||
> extends SaleorWebApiWebhook<TPayload, SyncWebhookInjectedContext<TEvent>> { | ||
TEvent extends SyncWebhookEventType = SyncWebhookEventType, | ||
> extends SaleorWebApiWebhook<TPayload> { | ||
readonly event: TEvent; | ||
|
||
protected readonly eventType = "sync" as const; | ||
|
||
protected extraContext = { | ||
buildResponse: buildSyncWebhookResponsePayload, | ||
}; | ||
|
||
constructor(configuration: WebhookConfig<TEvent>) { | ||
super(configuration); | ||
|
||
this.event = configuration.event; | ||
} | ||
|
||
createHandler(handlerFn: WebApiSyncWebhookHandler<TPayload, TEvent>): WebApiHandler { | ||
createHandler(handlerFn: WebApiSyncWebhookHandler<TPayload>): WebApiHandler { | ||
return super.createHandler(handlerFn); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,35 +1,26 @@ | ||
import { NextApiHandler } from "next"; | ||
|
||
import { SyncWebhookInjectedContext } from "@/handlers/shared"; | ||
import { buildSyncWebhookResponsePayload } from "@/handlers/shared/sync-webhook-response-builder"; | ||
import { SyncWebhookEventType } from "@/types"; | ||
|
||
import { NextJsWebhookHandler, SaleorWebhook, WebhookConfig } from "./saleor-webhook"; | ||
|
||
export type NextJsSyncWebhookHandler< | ||
TPayload, | ||
TEvent extends SyncWebhookEventType = SyncWebhookEventType | ||
> = NextJsWebhookHandler<TPayload, SyncWebhookInjectedContext<TEvent>>; | ||
export type NextJsSyncWebhookHandler<TPayload> = NextJsWebhookHandler<TPayload>; | ||
|
||
export class SaleorSyncWebhook< | ||
TPayload = unknown, | ||
TEvent extends SyncWebhookEventType = SyncWebhookEventType | ||
> extends SaleorWebhook<TPayload, SyncWebhookInjectedContext<TEvent>> { | ||
TEvent extends SyncWebhookEventType = SyncWebhookEventType, | ||
> extends SaleorWebhook<TPayload> { | ||
readonly event: TEvent; | ||
|
||
protected readonly eventType = "sync" as const; | ||
|
||
protected extraContext = { | ||
buildResponse: buildSyncWebhookResponsePayload, | ||
}; | ||
|
||
constructor(configuration: WebhookConfig<TEvent>) { | ||
super(configuration); | ||
|
||
this.event = configuration.event; | ||
} | ||
|
||
createHandler(handlerFn: NextJsSyncWebhookHandler<TPayload, TEvent>): NextApiHandler { | ||
createHandler(handlerFn: NextJsSyncWebhookHandler<TPayload>): NextApiHandler { | ||
return super.createHandler(handlerFn); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: we are going to remove
ctx
orbuildResponse
fromctx
? I'm asking as in the beginning of changeset it seems like we are going to removectx
completely 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intend to remove only buildResponse, ctx stays
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If yes - I would rephrase:
to