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

support relative paths to application_url for declarative webhooks #3402

Closed
wants to merge 2 commits into from

Conversation

alexanderMontague
Copy link
Contributor

@alexanderMontague alexanderMontague commented Feb 8, 2024

WHY are these changes introduced?

Closes https://github.com/Shopify/develop-app-management/issues/1599

WHAT is this pull request doing?

  • adds support for declarative webhook uris that are relative to the application_url
    ex:
application_url = "https://my-app-url.com"

[[webhooks.subscriptions]]
topics = ["products/create", "products/update"]
uri = "/products" # https://my-app-url.com/products
  • this is especially useful when using app dev as the app url changes every time

  • this also transforms the config on app config link, and we simplify like we do with the topics array
    ex. if you have

application_url = "https://my-app-url.com"

[[webhooks.subscriptions]]
topics = ["products/create"]
uri = "https://my-app-url.com/products"

[[webhooks.subscriptions]]
topics = ["orders/create"]
uri = "https://my-app-url.com/orders"

and link, we will simplify the config to

application_url = "https://my-app-url.com"

[[webhooks.subscriptions]]
topics = ["products/create"]
uri = "/products"

[[webhooks.subscriptions]]
topics = ["orders/create"]
uri = "/orders"

  • There is also nothing stopping anyone from creating a bad relative path like
application_url = "https://my-app-url.com"

[[webhooks.subscriptions]]
topics = ["products/create"]
uri = "//////products" # https://my-app-url.com//////products
  • As technically to https spec, this is still a valid URL. We could parse out extra forward slashes, but I think a developer would realize this once they see it in their TOML or on the dashboard
  • The application_url cannot end in a forward slash so this protects against this on the app url side

How to test your changes?

  • create an app and opt into the versioned config and declarative webhooks betas (both app and dev shop)
  • create some webhook subscriptions with a relative path
  • deploy the version
  • go to the dashboard, you should see the full URL
  • you can also link and make sure the toml does not change (unless it is this simplification case like mentioned)

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've made sure that any changes to dev or deploy have been reflected in the internal flowchart.

@alexanderMontague alexanderMontague force-pushed the declarative_webhooks_relative_path branch from caa3692 to b9f0614 Compare February 8, 2024 16:37
Copy link
Contributor

github-actions bot commented Feb 8, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
73.92% (+0.04% 🔼)
6602/8931
🟡 Branches
70.7% (+0.09% 🔼)
3204/4532
🟡 Functions
73.42% (-0.01% 🔻)
1732/2359
🟡 Lines
74.94% (+0.04% 🔼)
6225/8307

Test suite run success

1584 tests passing in 738 suites.

Report generated by 🧪jest coverage report action from d3daf82

@alexanderMontague alexanderMontague force-pushed the declarative_webhooks_relative_path branch from b9f0614 to 9a97afa Compare February 8, 2024 16:57
@alexanderMontague alexanderMontague marked this pull request as ready for review February 8, 2024 16:58
Copy link
Contributor

github-actions bot commented Feb 8, 2024

We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** 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.

Copy link
Contributor

@gonzaloriestra gonzaloriestra left a comment

Choose a reason for hiding this comment

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

The code LGTM, but I'm not sure about the approach as I commented here.

packages/app/src/cli/models/app/loader.ts Show resolved Hide resolved
Copy link
Contributor

@gracejychang gracejychang left a comment

Choose a reason for hiding this comment

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

some minor comments, but otherwise everything looks good to me!


// Then
expect(result).toMatchObject({
webhooks: {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should we check that application_url is what's to be expected as well?

Copy link
Contributor

This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action.
→ If there's no activity within a week, then a bot will automatically close this.
Thanks for helping to improve Shopify's dev tooling and experience.

@alexanderMontague
Copy link
Contributor Author

wip

Copy link
Contributor

This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action.
→ If there's no activity within a week, then a bot will automatically close this.
Thanks for helping to improve Shopify's dev tooling and experience.

@alexanderMontague
Copy link
Contributor Author

wip

@alexanderMontague
Copy link
Contributor Author

#3948

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.

4 participants