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

Make it easier to get started with webhooks #842

Merged
merged 6 commits into from
Sep 17, 2024

Conversation

byrichardpowell
Copy link
Contributor

WHY are these changes introduced?

We want to make it easier to get started with declarative webhooks

WHAT is this pull request doing?

  1. Provide the config for mandatory webhook topics
  2. Add a product create webhook, so partners can see a webhook trigger when pressing "generate a product"
  3. Separate each webhook topic into it's own URL

Test this PR

shopify app init --template=https://github.com/Shopify/shopify-app-template-remix#declarative-webhooks
  1. Run npm run dev. Do you see a message like this: webhooks │ APP_UNINSTALLED webhook delivered
  2. Click generate a product in the UI. Does the product create webhook trigger?
  3. Does the webhook config match the routes correctly?
  4. Does the code in each webhook make sense?

Checklist

  • I have made changes to the README.md file and other related documentation, if applicable
  • I have added an entry to CHANGELOG.md
  • I'm aware I need to create a new release when this PR is merged

@byrichardpowell byrichardpowell requested a review from a team as a code owner September 10, 2024 20:57
@byrichardpowell byrichardpowell force-pushed the declarative-webhooks branch 2 times, most recently from 3499cce to 26c2922 Compare September 11, 2024 13:07
1. Providing the config for mandatory webhook topics
2. Adding a product create webhook, so partners can see a webhook trigger when pressing "generte a product"
3. Separating each webhook topic into it's own URL
Copy link
Contributor

@lizkenyon lizkenyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A while back we had talked about using snippets as a way to share common code patterns with developers, and I feel like this could be a good use case!

I would like to confirm with the webhook team, but I am thinking for the privacy webhooks we want to be able to process them even if the app is uninstalled/has no session.

app/routes/webhooks.shop.redact.tsx Show resolved Hide resolved
app/routes/webhooks.customers.data_request.tsx Outdated Show resolved Hide resolved
Copy link

@cdarne cdarne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great way to tidy all the webhooks handling! Most of the webhook endpoints should not rely on a session or admin to be set, as the app could be already uninstalled for app/uninstall and compliance webhooks.

Ideally, we'd like to encourage receiving webhooks and respond ASAP and then do the processing asynchronously or using a job queue. (I understand that's probably out of the scope of the template)


// Webhook requests can trigger after an app is uninstalled.
// If the app is already uninstalled, the session may be undefined.
if (session) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're comment is right: the session might not be defined when processing a webhook request. From what I can understand, authenticate.webhook will first validate the webhook origin (by checking the HMAC signature from the headers) and then it will try to load an offline session and admin. But this can fail so we can only count on the shop, payload to be defined here.

TLDR: you can drop the session check IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are effectively using the session check here to say "Did we already process this?". Using shop, I think we'd need to do something like this:

  const { shop } = await authenticate.webhook(request);

  // Webhook requests can trigger after an app is uninstalled.
  // If the app is already uninstalled, the session may be undefined.
  try {
    db.session.deleteMany({ where: { shop } });
  } catch (error) {
    console.error(error);
  }

Try/catch is the recommended way in Prisma to delete without raising exceptions if the record exists. I don't think try/catch is quite as nice.

WDYT @lizkenyon & @cdarne ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for this case it makes sense to check for the session!

We could add another sentence for clarity if we wanted 🤷

// Webhook requests can trigger after an app is uninstalled. 
// If the app is already uninstalled, the session may be undefined, which means it has already been deleted.

app/routes/webhooks.customers.data_request.tsx Outdated Show resolved Hide resolved
app/routes/webhooks.shop.redact.tsx Show resolved Hide resolved
@byrichardpowell
Copy link
Contributor Author

@lizkenyon @cdarne please can I get a second review?

Note i also updated the README to steer partners towards app-specific webhooks

@byrichardpowell byrichardpowell merged commit 3584ecf into main Sep 17, 2024
5 checks passed
@byrichardpowell byrichardpowell deleted the declarative-webhooks branch September 17, 2024 14:20
github-actions bot pushed a commit that referenced this pull request Sep 17, 2024
Make it easier to get started with webhooks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants