Skip to content
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

Merged
merged 2 commits into from
Feb 27, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions .changeset/new-paws-win.md
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
Copy link
Member

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 or buildResponse from ctx? I'm asking as in the beginning of changeset it seems like we are going to remove ctx completely 🤔

Copy link
Member Author

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

Copy link
Member

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:

Removed `ctx` parameter from SyncWebhookHandler and replace with standalone `buildSyncWebhookResponsePayload` function

to

Removed `buildResponse` parameter from SyncWebhookHandler `ctx` and replace with standalone `buildSyncWebhookResponsePayload` function

)
{

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";

Expand All @@ -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",
);
});

Expand All @@ -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",
Copy link
Member

Choose a reason for hiding this comment

The 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? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The 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",
);
});
});
Expand Down Expand Up @@ -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);
Expand Down
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
Expand Up @@ -16,21 +16,21 @@ export type WebhookConfig<Event = AsyncWebhookEventType | SyncWebhookEventType>
GenericWebhookConfig<AwsLambdaHandlerInput, Event>;

/** Function type provided by consumer in `SaleorWebApiWebhook.createHandler` */
export type AwsLambdaWebhookHandler<TPayload = unknown, TExtras = {}> = (
export type AwsLambdaWebhookHandler<TPayload = unknown> = (
event: AwsLambdaHandlerInput,
context: Context,
ctx: WebhookContext<TPayload> & TExtras
ctx: WebhookContext<TPayload>,
) => Promise<APIGatewayProxyStructuredResultV2> | APIGatewayProxyStructuredResultV2;

export abstract class SaleorWebApiWebhook<
TPayload = unknown,
TExtras extends Record<string, unknown> = {}
> extends GenericSaleorWebhook<AwsLambdaHandlerInput, TPayload, TExtras> {
export abstract class SaleorWebApiWebhook<TPayload = unknown> extends GenericSaleorWebhook<
AwsLambdaHandlerInput,
TPayload
> {
/**
* Wraps provided function, to ensure incoming request comes from registered Saleor instance.
* Also provides additional `context` object containing typed payload and request properties.
*/
createHandler(handlerFn: AwsLambdaWebhookHandler<TPayload, TExtras>): AWSLambdaHandler {
createHandler(handlerFn: AwsLambdaWebhookHandler<TPayload>): AWSLambdaHandler {
return async (event, context) => {
const adapter = new AwsLambdaAdapter(event, context);
const prepareRequestResult = await super.prepareRequest<AwsLambdaAdapter>(adapter);
Expand All @@ -41,7 +41,6 @@ export abstract class SaleorWebApiWebhook<

debug("Incoming request validated. Call handlerFn");
return handlerFn(event, context, {
...(this.extraContext ?? ({} as TExtras)),
...prepareRequestResult.context,
});
};
Expand Down
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";

Expand Down Expand Up @@ -41,17 +41,15 @@ describe("Web API SaleorSyncWebhook", () => {
},
});

const handler = vi
.fn<WebApiSyncWebhookHandler<Payload>>()
.mockImplementation((_request, ctx) => {
const responsePayload = ctx.buildResponse({
lines: [{ tax_rate: 8, total_net_amount: 10, total_gross_amount: 1.08 }],
shipping_price_gross_amount: 2,
shipping_tax_rate: 8,
shipping_price_net_amount: 1,
});
return new Response(JSON.stringify(responsePayload), { status: 200 });
const handler = vi.fn<WebApiSyncWebhookHandler<Payload>>().mockImplementation(() => {
const responsePayload = buildSyncWebhookResponsePayload<"ORDER_CALCULATE_TAXES">({
lines: [{ tax_rate: 8, total_net_amount: 10, total_gross_amount: 1.08 }],
shipping_price_gross_amount: 2,
shipping_tax_rate: 8,
shipping_price_net_amount: 1,
});
return new Response(JSON.stringify(responsePayload), { status: 200 });
});

const saleorSyncWebhook = new SaleorSyncWebhook<Payload>(webhookConfiguration);

Expand All @@ -70,7 +68,7 @@ describe("Web API SaleorSyncWebhook", () => {
shipping_price_gross_amount: 2,
shipping_tax_rate: 8,
shipping_price_net_amount: 1,
})
}),
);
});

Expand Down
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,7 +1,7 @@
import { createMocks } from "node-mocks-http";
import { afterEach, describe, expect, it, vi } from "vitest";

import { WebhookError } from "@/handlers/shared";
import { buildSyncWebhookResponsePayload, WebhookError } from "@/handlers/shared";
import { SaleorWebhookValidator } from "@/handlers/shared/saleor-webhook-validator";
import { MockAPL } from "@/test-utils/mock-apl";

Expand Down Expand Up @@ -31,7 +31,7 @@ describe("Next.js SaleorSyncWebhook", () => {
expect(manifest).toEqual(
expect.objectContaining({
targetUrl: `${baseUrl}/${validSyncWebhookConfiguration.webhookPath}`,
})
}),
);
});

Expand Down Expand Up @@ -66,8 +66,8 @@ describe("Next.js SaleorSyncWebhook", () => {
});

const saleorSyncWebhook = new SaleorSyncWebhook(validSyncWebhookConfiguration);
const testHandler: NextJsWebhookHandler = vi.fn().mockImplementation((_req, res, ctx) => {
const responsePayload = ctx.buildResponse({
const testHandler: NextJsWebhookHandler = vi.fn().mockImplementation((_req, res) => {
const responsePayload = buildSyncWebhookResponsePayload({
lines: [{ tax_rate: 8, total_net_amount: 10, total_gross_amount: 1.08 }],
shipping_price_gross_amount: 2,
shipping_tax_rate: 8,
Expand All @@ -88,7 +88,7 @@ describe("Next.js SaleorSyncWebhook", () => {
shipping_price_gross_amount: 2,
shipping_tax_rate: 8,
shipping_price_net_amount: 1,
})
}),
);
});

Expand Down
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);
}
}
7 changes: 0 additions & 7 deletions src/handlers/shared/saleor-webhook.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
import { SyncWebhookEventType } from "@/types";

import { AuthData } from "../../APL";
import { buildSyncWebhookResponsePayload } from "./sync-webhook-response-builder";

export const WebhookErrorCodeMap: Record<SaleorWebhookError, number> = {
OTHER: 500,
Expand Down Expand Up @@ -58,10 +55,6 @@ export type WebhookContext<TPayload> = {
schemaVersion: number | null;
};

export type SyncWebhookInjectedContext<TEvent extends SyncWebhookEventType> = {
buildResponse: typeof buildSyncWebhookResponsePayload<TEvent>;
};

export type FormatWebhookErrorResult = {
code: number;
body: string;
Expand Down
2 changes: 1 addition & 1 deletion src/handlers/shared/sync-webhook-response-builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,5 +167,5 @@ export type SyncWebhookResponsesMap = {
* Identity function, but it works on Typescript level to pick right payload based on first param
*/
export const buildSyncWebhookResponsePayload = <E extends SyncWebhookEventType>(
payload: SyncWebhookResponsesMap[E]
payload: SyncWebhookResponsesMap[E],
): SyncWebhookResponsesMap[E] => payload;