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

Add support for Metafields to webhooks #5167

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rsperko
Copy link

@rsperko rsperko commented Jan 7, 2025

WHY are these changes introduced?

Add support for metafield identifiers in declarative webhook subscriptions, allowing apps to obtain metafields in their webhook payloads for their subscriptions. Core support was added here, documentation is added here, this adds cli support.

Applicable CLI support is being added here

WHAT is this pull request doing?

How to test your changes?

Post-release steps

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

Copy link
Contributor

github-actions bot commented Jan 7, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.16% (+0.02% 🔼)
8876/11809
🟡 Branches
70.35% (+0.02% 🔼)
4323/6145
🟡 Functions
75.08% (+0.02% 🔼)
2320/3090
🟡 Lines
75.72% (+0.02% 🔼)
8389/11079
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / app_config_webhook.ts
100%
89.74% (-10.26% 🔻)
100% 100%
🟢
... / app-event-watcher.ts
93.83% (-1.23% 🔻)
86.49% (-2.7% 🔻)
90.48% 98.61%
🟢
... / ConcurrentOutput.tsx
98.36% (-1.64% 🔻)
88% (-4% 🔻)
100%
98.33% (-1.67% 🔻)

Test suite run success

2014 tests passing in 906 suites.

Report generated by 🧪jest coverage report action from eea7761

@rsperko rsperko force-pushed the rsperko/webhooks-metafield-support branch 3 times, most recently from d20c9a8 to 4be2a60 Compare January 9, 2025 18:04
@@ -76,3 +87,10 @@ function validateSubscriptions(webhookConfig: WebhooksConfig) {
}
}
}

Copy link
Author

Choose a reason for hiding this comment

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

@shauns I would like to get your early opinion on api version checking like this? I half wanted to add a general "api changes support" where we have a toml (?) file that we define minimum versions for changes and allow checking for that, but this is really the only place that currently needs this sort of support.

Any thoughts are appreciated.

Copy link
Contributor

Choose a reason for hiding this comment

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

The big picture for module schemas is they move to being only defined in the server -- some modules do this already. Webhook subscriptions clearly doesn't -- so I think keeping this logic self-contained and simple and only found in this one, later removed, place, feels pragmatic.

@rsperko rsperko force-pushed the rsperko/webhooks-metafield-support branch 2 times, most recently from 879335c to 3caf63e Compare January 10, 2025 17:11
@rsperko rsperko force-pushed the rsperko/webhooks-metafield-support branch from 3caf63e to eea7761 Compare January 10, 2025 20:18
@rsperko rsperko marked this pull request as ready for review January 10, 2025 22:08
@rsperko rsperko requested a review from a team as a code owner January 10, 2025 22:08
Copy link
Contributor

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

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.

2 participants