remove built in webhook response builder #413
Conversation
🦋 Changeset detectedLatest commit: c04f993 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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #413 +/- ##
==========================================
- Coverage 83.40% 83.34% -0.07%
==========================================
Files 98 98
Lines 3441 3428 -13
Branches 588 585 -3
==========================================
- Hits 2870 2857 -13
Misses 560 560
Partials 11 11 ☔ View full report in Codecov by Sentry. |
.changeset/new-paws-win.md
Outdated
| "@saleor/app-sdk": major | ||
| --- | ||
|
|
||
| Removed `ctx` parameter from SyncWebhookHandler and replace with standalone `buildSyncWebhookResponsePayload` function |
There was a problem hiding this comment.
Question: what about AsyncWebhookHandler? or it didn't have buildResponse?
There was a problem hiding this comment.
it didnt, we do not expect anything specific to be returned from async
in another PR i will introduce a building utility to return errors but I will detach it from handler context
| import { buildSyncWebhookResponsePayload } from "@saleor/app-sdk/handlers/shared"; | ||
|
|
||
| new SaleorSyncWebhook(...).createHandler( | ||
| req, res, ctx |
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
I intend to remove only buildResponse, ctx stays
There was a problem hiding this comment.
If yes - I would rephrase:
Removed `ctx` parameter from SyncWebhookHandler and replace with standalone `buildSyncWebhookResponsePayload` functionto
Removed `buildResponse` parameter from SyncWebhookHandler `ctx` and replace with standalone `buildSyncWebhookResponsePayload` function| @@ -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.
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.
Yea, dont know why it didnt work on pre-commit on previous prs
There was a problem hiding this comment.
Sorry I do know - I updated prettier and it's defaults changed
lkostrowski
force-pushed
the
refactor-build-sync-response
branch
from
679066d to
c04f993
Compare
No description provided.