Skip to content

Conversation

@georgge293
Copy link

@georgge293 georgge293 commented Nov 5, 2025

closes https://github.com/shop/issues-event-foundations/issues/518

WHY are these changes introduced?

Adds support for the name field on webhook subscriptions when registered in the app's shopify.app.toml

WHAT is this pull request doing?

  • Adds optional name field to webhook subscription TypeScript types
  • Adds validation for the name field in webhook subscription schemas (must be a non-empty string if provided)
  • Adds test coverage for webhook subscriptions with the name attribute

Name validation:

  • Field is optional
  • Must be a non-empty string if provided

How to test your changes

  1. Create a webhook subscription in your shopify.app.toml with a name field while running local app development in dev dash:

Example:

[[webhooks.subscriptions]]
name = "products-create"
topics = ["products/create"]
uri = "https://example.com/webhooks"
  1. Run shopify app deploy and verify it accepts the configuration
  2. Try with an empty name = "" and verify validation rejects it
  3. Run the test suite: pnpm vitest loader.test.ts --run

Tophat:

  • Tophatted changes locally verifying that the min and max char validations are working

@georgge293 georgge293 requested a review from a team as a code owner November 5, 2025 16:39
@github-actions

This comment has been minimized.

@georgge293 georgge293 requested a review from a team as a code owner November 5, 2025 17:00
@github-actions
Copy link
Contributor

github-actions bot commented Nov 5, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 79.28% 13607/17164
🟡 Branches 73.14% 6651/9094
🟡 Functions 79.36% 3507/4419
🟡 Lines 79.64% 12852/16138
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / ConcurrentOutput.tsx
98.36% (-1.64% 🔻)
92% (-4% 🔻)
100%
98.33% (-1.67% 🔻)

Test suite run success

3359 tests passing in 1372 suites.

Report generated by 🧪jest coverage report action from fc13dbb

@georgge293 georgge293 force-pushed the add-webhook-name-field branch from ad0e53b to 88db281 Compare November 5, 2025 19:31
@georgge293
Copy link
Author

I have signed the CLA!

@georgge293 georgge293 force-pushed the add-webhook-name-field branch from 88db281 to 2c76c59 Compare November 5, 2025 19:44
@georgge293 georgge293 force-pushed the add-webhook-name-field branch from 2c76c59 to e6e5eb6 Compare November 5, 2025 20:23
@georgge293
Copy link
Author

Note: Shopify core changes must be deployed first before the CLI or adding the name field would fail to be recognized and error

@plvaldes
Copy link
Contributor

plvaldes commented Nov 6, 2025

Note: Shopify core changes must be deployed first before the CLI or adding the name field would fail to be recognized and error

@georgge293 This PR is already approved but the core changes won't be deployed until after BFCM. I see some chances this PR is still accidentally merged. Which might be fine because afaik there won't be more CLI releases before BFCM, but core pending deploys could take a few days to deploy after BFCM and we should be careful to not release the CLI until that specific PR is deployed if we don't want to release a broken feature.

I may be a bit paranoid here, but what about moving this PR back to draft? You would keep the approvals and prevent an accidental merge from someone who hasn't read your comment on the core changes dependency

@georgge293
Copy link
Author

Note: Shopify core changes must be deployed first before the CLI or adding the name field would fail to be recognized and error

@georgge293 This PR is already approved but the core changes won't be deployed until after BFCM. I see some chances this PR is still accidentally merged. Which might be fine because afaik there won't be more CLI releases before BFCM, but core pending deploys could take a few days to deploy after BFCM and we should be careful to not release the CLI until that specific PR is deployed if we don't want to release a broken feature.

I may be a bit paranoid here, but what about moving this PR back to draft? You would keep the approvals and prevent an accidental merge from someone who hasn't read your comment on the core changes dependency

Sounds good, will do. Thanks!

@georgge293 georgge293 marked this pull request as draft November 6, 2025 18:48
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.

5 participants