[WIP] [Feature] Pass webhook id to handlers #606
Conversation
|
Hey @jbwyme, thanks for adding this! Checks are failing on Can you please sign Shopify’s CLA. Also are you able to add a test? |
| if (!webhookId) { | ||
| missingHeaders.push(ShopifyHeader.WebhookId); | ||
| } |
There was a problem hiding this comment.
Since this header isn't strictly necessary for validation, I wonder if we want to fail on it missing. That being said, we should always have the header so I suppose there's no harm in checking 🙂.
There was a problem hiding this comment.
It's optional in the WebhookHandlerFunction type, so it probably shouldn't be mandatory here. The existing tests aren't passing because of this check
There was a problem hiding this comment.
My 2c: It's optional in the type for backwards compatibility but the expected behavior is the header always exists. Therefore, I'd argue this is more correct. However, it's a minor nit and it probably doesn't matter much either way. Happy to go either direction!
|
Thanks for the review! I got side tracked but I'll finish out this PR today. |
|
I think the code looks good, but we seem to have a few tests breaking because they're not setting up the id header in the mocks, could you please fix those so we can merge? |
|
That's what I get for trying to do the whole PR in the github editor :D |
WHY are these changes introduced?
Fixes #370
WHAT is this pull request doing?
Grabs the webhook id from the 'X-Shopify-Webhook-Id' header and passes it to the webhook handlers.
Type of change
Checklist