New handlers: refactored, Web API and AWS Lambda support #380

witoszekdev · 2025-01-14T17:58:11Z

Description

This PR introduces new handlers for Fetch Web API and AWS Lambda and refactors existing code so that it can easily allow adding new platforms in the future.

After the change handlers code is splits them into 3 parts:

Platform Adapter - class that is responsible of:
- parsing received requests from specific platform format (e.g. Next.js request, Web API Request) into a common format
- and preparing response from common format into platform specific format (e.g. Next.js response, Web API Response)
Handler use cases - class that represents business logic specific to handler, has a single method that returns a result
- e.g. register use case: readers all required headers from request, validates it, saves to apl, returns success or failure
Handler factory method - function that creates a handler instance with all dependencies injected, each platform has separate folder, for example:
- src/handlers/nextjs - For handlers that use Next.js platform adapter
- src/handlers/fetch-api - For handlers that use Fetch API platform adapter

This way we'll be able to easily add new platforms and handlers without changing existing code.

Currently in this PR:

Next.js - existing implementation, shouldn't be changed
Web API - new implementation
AWS Lambda event - new implementation

Examples and docs

Usage examples:

https://github.com/witoszekdev/saleor-app-hono-aws-lambda-template
https://github.com/witoszekdev/saleor-app-hono-deno-template
https://github.com/witoszekdev/saleor-app-hono-cf-pages-template
saleor/saleor-app-template#267

Docs:

saleor/saleor-docs#1437

Breaking changes?

SaleorWebhook accepts onError and formatErrorResponse parameters callbacks, now they won't pass res object in Next.js runtime, since these methods weren't supposed to send responses

changeset-bot · 2025-01-14T17:58:15Z

🦋 Changeset detected

Latest commit: 534b64f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@saleor/app-sdk	Minor

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

easier testing

lkostrowski

It would be easier for me to review if I already see docs with examples. In case of SDK public API is more important than the code

lkostrowski · 2025-01-24T15:21:35Z

.changeset/fluffy-cheetahs-sleep.md

@@ -0,0 +1,5 @@
+---
+"@saleor/app-sdk": minor


please make it major since it has breaking changes

lkostrowski · 2025-01-24T15:21:47Z

.changeset/fluffy-cheetahs-sleep.md

+"@saleor/app-sdk": minor
+---
+
+Added `handlers/fetch-api` which adds support for frameworks that use [Fetch API](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API)


please remember to add full changeset and docs

saleor/saleor-docs#1437

lkostrowski · 2025-01-24T15:24:05Z

package.json

+    "./handlers/next-app-router": {
+      "types": "./handlers/fetch-api/index.d.ts",
+      "import": "./handlers/fetch-api/index.mjs",
+      "require": "./handlers/fetch-api/index.js"
+    },


won't it have the same names as fetch api? It would be confusing, probably better to also add alias on export

so next-app-router file can include export { FetchApi as NextAppRouterWebhook } from '../fetch-api' or something like this

would be easier if we see docs at this point

This is fine, since exported names do not reference Fetch API in any way, names are the same as in Next.js handlers except they are in a different folder. Our exports are:

createAppRegisterHandler

createManifestHandler

createProtectedHandler

SaleorSyncWebhook

SaleorAsyncWebhook

lkostrowski

another partial review ;)

lkostrowski · 2025-01-27T07:28:22Z

src/fetch-middleware/index.ts

what is "fetch middleware"? If this is a middleware using Fetch Request API, I don't think we should call it that way. Maybe just "middleware"?

lkostrowski · 2025-01-27T07:30:32Z

src/fetch-middleware/types.ts

@@ -0,0 +1,5 @@
+export type SaleorRequest = Request & { context?: Record<string, any> };


probably you should avoid any in favor of unknown

lkostrowski · 2025-01-27T07:35:03Z

src/fetch-middleware/with-auth-token-required.ts

please add tests to all middlewares

lkostrowski · 2025-01-27T07:37:39Z

src/fetch-middleware/with-registered-saleor-domain-header.ts

+
+const debug = createFetchMiddlewareDebug("withRegisteredSaleorDomainHeader");
+
+export const withRegisteredSaleorDomainHeader: FetchMiddleware = (handler) => async (request) => {


we are actually checking "saleorApiUrl", not domain, and it 1.0.0 we will drop domain checking

lkostrowski · 2025-01-27T07:41:09Z

src/fetch-middleware/with-registered-saleor-domain-header.ts

+    );
+  }
+
+  const authData = await saleorApp?.apl.get(saleorApiUrl);


maybe can we extract this? Middleware checks header but also checks if auth data exists. And doesnt do anything with it. It doesnt event attach it to request, so it's cached

lkostrowski · 2025-01-27T07:43:02Z

src/fetch-middleware/with-saleor-domain-present.ts

+
+const debug = createFetchMiddlewareDebug("withSaleorDomainPresent");
+
+export const withSaleorDomainPresent: FetchMiddleware = (handler) => async (request) => {


we can remove this, in v1 we can remove domain header, so no need to check this

src/handlers/actions/manifest-action-handler.ts

lkostrowski · 2025-01-27T07:53:58Z

src/handlers/actions/register-action-handler.ts

+    const checksToRun = [
+      this.adapterMiddleware.withMethod(["POST"]),
+      this.adapterMiddleware.withSaleorDomainPresent(),
+    ];


Im not sure if middleware should work as "check" only. If you check against e.g. existence of AuthData, you can pass Request and middleware can enrich it with context - eg attach AuthData there

I don't think modifying Request object is a good idea. It would be like adding new properties to globals like Array or Window 😬 These objects are meant to be immutable.

Adding context is usually done by web frameworks that use Fetch API by creating new Context object and passing it everywhere, or by using AsyncLocalStorage. Example in Hono.

We could do this, but then we would need to have some context mechanism of our own. We could also use AsyncLocalStorage, but this is also problematic since runtimes like Cloudflare Workers, or Deno require enabling Node compatibility layer to do this :/

Modifying request isn't anything new. It's not about modifying global, but instance. It's not about Array.prototype.foo by (new Array()).foo

Context you mentioned is a platform context.
What I mean is request context. It can be achieved several ways, eg with async_hooks, modifying request or adding another argument (like createWebhookHandler(req, res, ctx)). Or how TRPC middleware works.

You can also read eg https://expressjs.com/en/guide/using-middleware.html

What I want to achieve is to allow middleware to attach data for next middlewares or handlers and compose the rich request in the end.

For example, if your handler fetches AuthData, it can attach it to request (pass to next step), instead of fetching it again in next middleware/handler

OK, I think we are talking about different things here and maybe the name middleware in this place is wrong and confusing 😅

These "middlewares" do not have any data we would want to pass as context, since they don't do any asynchronous actions.

This specific register action has a concept of "context" in its config, where you can attach to different events, e.g.:

/** * Run right after Saleor calls this endpoint */ onRequestStart?( request: RequestType, context: { authToken?: string; saleorDomain?: string; saleorApiUrl?: string; respondWithError: CallbackErrorHandler; } ): Promise<void>;

Config shape is the same as previous Next.js actions in app-sdk, it just has different Request object type depending on platform

createProtectedHandler also has its own context:

export type ProtectedHandlerContext = { baseUrl: string; authData: AuthData; user: TokenUserPayload; };

Webhook handlers also have context (no change from previous public API):

export type SaleorWebhookHandler<TPayload = unknown, TExtras = {}> = ( req: Request, ctx: WebhookContext<TPayload> & TExtras ) => Response | Promise<Response>;

As for fetch-middleware, please see my comment: #380 (comment).

lkostrowski · 2025-01-27T07:55:34Z

src/handlers/actions/register-action-handler.ts

+      // TODO: We should strictly require `saleorApiUrl` instead of
+      // relying on `saleor-domain` that is deprecated


comment is stale

why? the comment is still valid because we are not doing strict check on saleorApiUrl for compatibility reasons (I didn't change bussiness logic, rather I just copied our old code and adapted it to work with our new adapter abstraction).

We can make another breaking change and just drop the Saleor-Domain usage altogether imho

witoszekdev · 2025-01-27T10:30:25Z

@lkostrowski Public API usage doesn't change, the change made to existing Next.js handlers are meant to be drop-in and non-breaking. New handlers for Web API and Lambda have the same signature as Next.js handlers, they only differ when accepting a request object.

The only breaking change is removing one parameter, which I think was a mistake in our API design, since we were passing res object from Next.js in place where we didn't want API consumers to return a custom response, but rather to return an object that is then mapped by app-sdk to a response. Calling res multiple times is not supported.

I've prepared examples here:

And docs here: saleor/saleor-docs#1437

witoszekdev · 2025-01-27T10:53:13Z

@lkostrowski one more thing: fetch-middleware was a leftover from old PR that I resumed work on and I actually didn't intend on including these changes. Instead I've created PlatformAdapterMiddleware class that does most of the features of these middlewares.

I'm also not sure if we even want to add these middlewares to begin with? They are not used in other places in app-sdk. Previously we had them because they were passed to retes.

I also don't think these are used in any of our apps, since most of them use higher level abstractions by using /handlers. I think this is a better approach, since using low-level middlewares could expose users of app-sdk to malicious requests if not all validations are used in their application code. I think it's much easier to rely on a handler that does all the validation for you.

codecov · 2025-01-27T11:31:25Z

Codecov Report

Attention: Patch coverage is 44.09654% with 857 lines in your changes missing coverage. Please review.

Project coverage is 62.71%. Comparing base (cd902b5) to head (534b64f).

Files with missing lines	Patch %	Lines
src/handlers/shared/protected-action-validator.ts	8.10%	136 Missing ⚠️
src/handlers/actions/register-action-handler.ts	76.78%	75 Missing ⚠️
.../handlers/platforms/aws-lambda/platform-adapter.ts	2.00%	49 Missing ⚠️
...c/handlers/platforms/fetch-api/platform-adapter.ts	2.00%	49 Missing ⚠️
src/handlers/shared/generic-saleor-webhook.ts	68.00%	40 Missing ⚠️
...middleware/with-registered-saleor-domain-header.ts	0.00%	38 Missing and 1 partial ⚠️
src/handlers/shared/saleor-webhook-validator.ts	81.16%	29 Missing ⚠️
...s/platforms/aws-lambda/create-protected-handler.ts	3.57%	27 Missing ⚠️
...aws-lambda/saleor-webhooks/saleor-async-webhook.ts	3.70%	26 Missing ⚠️
.../fetch-api/saleor-webhooks/saleor-async-webhook.ts	3.70%	26 Missing ⚠️
... and 28 more

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #380       +/-   ##
===========================================
- Coverage   73.37%   62.71%   -10.67%     
===========================================
  Files          79      116       +37     
  Lines        3061     4197     +1136     
  Branches      457      572      +115     
===========================================
+ Hits         2246     2632      +386     
- Misses        810     1544      +734     
- Partials        5       21       +16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2025-01-27T11:33:05Z

Released snapshot build with @dev tag in npm with version: 0.0.0-pr-20250127113254.

Install it with:

pnpm add @saleor/[email protected]

witoszekdev · 2025-01-28T16:02:09Z

This PR will be split into multiple smaller ones for easier review

witoszekdev added 22 commits March 8, 2024 10:47

Upgrade TypeScript

84a2d4b

Upgrade TS ESlint parser

7e1ca85

Add manifest handler for Edge Runtime

6ca0c67

Fix TS errors

37fd731

Add changeset

fa1164d

Add default export

362c1f6

Expose new folder

b2947b2

Add middlewares for FetchAPI, add Edge register handler

eca6911

Merge branch 'main' into add-edge-support

cd7a49a

Add Allow header for HTTP 405

4ddd53c

Rename next-edge -> fetch-api

09da9c2

Update changeset

d1dc948

Add shared interface, remove usage of "NextEdge"

9d34b5f

Add protected route handler

a4f75be

Add context to protected handler

8f9a334

Fix broken build

c4eb069

Fix exports in package.json

b0a237a

Add webhook handlers

9e5c059

Export missing functions, fix errors when handling Web api webhooks

98ace22

Fix web api webhook typing

1b87494

Add fallback for Request.url.protocol

d5e8785

Refactor handler into use case and adapters

32bec65

witoszekdev requested review from a team and lkostrowski January 14, 2025 17:58

witoszekdev changed the base branch from main to add-edge-support January 14, 2025 17:59

witoszekdev marked this pull request as draft January 14, 2025 17:59

witoszekdev requested a review from krzysztofzuraw January 14, 2025 18:10

witoszekdev added 2 commits January 15, 2025 13:24

Add AWS lambda adapter

69203aa

Update exports

bb36e97

witoszekdev added 13 commits January 21, 2025 13:32

Fix TS errors

ab1c269

Refactor protected handler validation

91562da

Add getRawBody handler

d52f28b

Rewrite webhook validation to use adapter

36d9088

Add common logic for Saleor webhook validation using adapter

f146bc3

Add webhok impl for fetch api

efd0f8d

Add generic webhook handler, add aws lambda webhook

9462deb

Add AWS Lambda Saleor webhook, update generic types, fix types web api

966defe

Remove unused file

dd575a1

Fix TS error

f7b1aec

Change webhook validator to not require adapter in constructor for

f9e0d3f

easier testing

Fix tests

3d09ac3

Add tests for request validator

6712040

witoszekdev added release dev tag and removed release dev tag labels Jan 24, 2025

witoszekdev requested review from lkostrowski and andrzejewsky January 24, 2025 14:39

lkostrowski requested changes Jan 24, 2025

View reviewed changes

lkostrowski reviewed Jan 27, 2025

View reviewed changes

witoszekdev added release dev tag and removed release dev tag labels Jan 27, 2025

Merge branch 'main' into refactor-handler-functions

adbbe1c

witoszekdev added release dev tag and removed release dev tag labels Jan 27, 2025

Remove aws-lambda runtime library

534b64f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New handlers: refactored, Web API and AWS Lambda support #380

New handlers: refactored, Web API and AWS Lambda support #380

witoszekdev commented Jan 14, 2025 •

edited

Loading

changeset-bot bot commented Jan 14, 2025 •

edited

Loading

lkostrowski left a comment

lkostrowski Jan 24, 2025

lkostrowski Jan 24, 2025

witoszekdev Jan 27, 2025

lkostrowski Jan 24, 2025

lkostrowski Jan 24, 2025

witoszekdev Jan 27, 2025

lkostrowski left a comment

lkostrowski Jan 27, 2025

lkostrowski Jan 27, 2025

lkostrowski Jan 27, 2025

lkostrowski Jan 27, 2025

lkostrowski Jan 27, 2025

lkostrowski Jan 27, 2025

lkostrowski Jan 27, 2025

witoszekdev Jan 27, 2025

lkostrowski Jan 27, 2025

witoszekdev Jan 27, 2025

lkostrowski Jan 27, 2025

witoszekdev Jan 27, 2025

witoszekdev commented Jan 27, 2025 •

edited

Loading

witoszekdev commented Jan 27, 2025 •

edited

Loading

codecov bot commented Jan 27, 2025 •

edited

Loading

github-actions bot commented Jan 27, 2025

witoszekdev commented Jan 28, 2025

		@@ -0,0 +1,5 @@
		export type SaleorRequest = Request & { context?: Record<string, any> };


		const debug = createFetchMiddlewareDebug("withRegisteredSaleorDomainHeader");

		export const withRegisteredSaleorDomainHeader: FetchMiddleware = (handler) => async (request) => {


		const debug = createFetchMiddlewareDebug("withSaleorDomainPresent");

		export const withSaleorDomainPresent: FetchMiddleware = (handler) => async (request) => {

		// TODO: We should strictly require `saleorApiUrl` instead of
		// relying on `saleor-domain` that is deprecated

New handlers: refactored, Web API and AWS Lambda support #380

Are you sure you want to change the base?

New handlers: refactored, Web API and AWS Lambda support #380

Conversation

witoszekdev commented Jan 14, 2025 • edited Loading

Description

Examples and docs

Breaking changes?

changeset-bot bot commented Jan 14, 2025 • edited Loading

🦋 Changeset detected

lkostrowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkostrowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

witoszekdev commented Jan 27, 2025 • edited Loading

witoszekdev commented Jan 27, 2025 • edited Loading

codecov bot commented Jan 27, 2025 • edited Loading

Codecov Report

github-actions bot commented Jan 27, 2025

witoszekdev commented Jan 28, 2025

witoszekdev commented Jan 14, 2025 •

edited

Loading

changeset-bot bot commented Jan 14, 2025 •

edited

Loading

witoszekdev commented Jan 27, 2025 •

edited

Loading

witoszekdev commented Jan 27, 2025 •

edited

Loading

codecov bot commented Jan 27, 2025 •

edited

Loading