Add new Adapter and Action interfaces, add new Next.js handler implementation, remove middlewares #401
Conversation
🦋 Changeset detectedLatest commit: 2e0b09d 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 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #401 +/- ##
==========================================
+ Coverage 73.07% 81.16% +8.09%
==========================================
Files 82 78 -4
Lines 3034 3143 +109
Branches 456 513 +57
==========================================
+ Hits 2217 2551 +334
+ Misses 812 587 -225
Partials 5 5 ☔ View full report in Codecov by Sentry. |
.changeset/tough-socks-tease.md
Outdated
| @@ -2,4 +2,6 @@ | |||
| "@saleor/app-sdk": major | |||
| --- | |||
|
|
|||
| Breaking change: Remove checking "domain" header from Saleor requests. It should be replaced with the "saleor-api-url" header. | |||
| Breaking change: Remove checking "saleor-domain" header from Saleor requests. It should be replaced with the "saleor-api-url" header. | |||
There was a problem hiding this comment.
But who should check saleor-api-url? app or sdk? If SDK, it's not a part of the changelog but something we should do
There was a problem hiding this comment.
Nope, this is something SDK does: we have dropped saleor-domain which was deprecated in favor of saleor-api-url header in our validation, thus making SDK incompatible with versions prior to 3.15. I've improved the description so that it's more clear.
package.json
Outdated
| "./handlers/actions": { | ||
| "types": "./handlers/actions/index.d.ts", | ||
| "import": "./handlers/actions/index.mjs", | ||
| "require": "./handlers/actions/index.js" | ||
| }, |
There was a problem hiding this comment.
what is the use case of this being exported? Why App would need that?
There was a problem hiding this comment.
If you're not using any of our adapters, you can re-implement them for your platform without forking app-sdk
There was a problem hiding this comment.
I think on 1.x better to not export more than we need. Every time we export something we must be sure it should be there, because its public api and everyone can build on top
Unless someone asks for, I'd prefer to export only platform specific logic, while we control the internals. The we can evolve into exposing this
There was a problem hiding this comment.
All right, I haven't though about that this way 😅
I'll remove internals export then 👍🏻
| const methodEagerResponse = this.adapterMiddleware.withMethod(["GET"]); | ||
|
|
||
| if (methodEagerResponse) { | ||
| return methodEagerResponse; | ||
| } |
There was a problem hiding this comment.
It returns an error response if validation failed, I've updated the name
|
|
||
| import { HTTPMethod, PlatformAdapterInterface } from "./generic-adapter-use-case-types"; | ||
|
|
||
| export class PlatformAdapterMiddleware<T> { |
There was a problem hiding this comment.
Maybe better name would be to mention what it does (validating requests), not that it uses adapter for it?
There was a problem hiding this comment.
👍 I've came up with SaleorRequestProcessor since it does both some validation and getting of headers
|
|
||
| export class ProtectedActionValidator<I> { | ||
| // Name left for backwards compatibility | ||
| private debug = createDebug("processProtectedHandler"); |
There was a problem hiding this comment.
We dont need this compatibility, we are introdcucing bc
.changeset/tough-socks-tease.md
Outdated
| Breaking change: Remove checking "domain" header from Saleor requests. It should be replaced with the "saleor-api-url" header. | ||
| Breaking change: SDK will no longer check `saleor-domain` header when validating Saleor requests, instead it will check `saleor-api-url` header. | ||
|
|
||
| This makes SDK incompatible with Saleor versions prior to 3.15. |
There was a problem hiding this comment.
We don't have to write that, I want SDK 1.x to drop support below 3.20
This PR adds new interfaces for writing handlers:
It replaces Next.js handlers with these new abstractions.
It removes deprecated process-protected-handler and process-saleor-webhook files, in favor of Validator classes.
It also removes middlewares and
retesdependency, since they are no longer used in handlers.Additional tests for existing usage were added: for example, fetching of JWKS when Saleor webhook validation has failed, webhook error handling.
TODO for next PR:
schemaVersionrequired in SaleorWebhook