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

Update webhook subscription URIs when running dev #4222

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

ozzyonfire
Copy link

WHY are these changes introduced?

WIP on #4209. Webhook subscriptions uris are not updated when running an app in dev.

WHAT is this pull request doing?

Recently, we got the ability to configure webhook subsciprions in the app.toml file. However, these uris are not updated on dev (only deployment). This means we need to deploy the app to get proper uris (for testing) which isn't the greatest DX.

I was only able to make small updates around the config file. I currently have the new URLs ready to be sent via graphql to the Partner API, but I couldn't find and documentation or schemas related to this api. I assume because this is an internal api?

How to test your changes?

I ended up forking from a stable branch, because I couldn't get the tests to pass in the main branch. All tests should still pass with pnpm test.

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

Contribution questions

I tried to contribute to this project following the documentation laid out in the repo, but I ran into a couple of weird issues. Maybe this is due to my setup - hoping to get some clarification.

I had to link a bunch of packages to their local monorepo projects, instead of relying on the version numbers in the package.json files. e.g. pnpm wouldn't install "@shopify/app": "3.64.0" since that version doesn't exist on npm. Instead I needed to use "@shopify/app": "link": "../app". Shouldn't these packages refer to their local monorepo versions? Maybe my pnpm isn't set up to do this?

I wasn't really sure what version of pnpm to use (see above comment). Maybe using something like corepack to make the version of pnpm consistent would be useful (or link it in the docs).

I was making good progress on this feature/enhancement, but then I ran into a part of the code that I was kind of flying blind - I tried using graphql-codegen to get types and some docs from the schema, but of course I don't have a _schema.graphql file for the business or partners apis. Is this something you could open up for contributors?

Regarding the branches, I also wasn't sure if we should be making the PRs go to main? Or what the default branch is? Like I mentioned, I got some syntax errors and type errors when forking main. When forking a stable branch, everything worked and all the tests passed right away. So I'm not even sure if I should try to merge back into main or into the stable branch - (which looked like I could automatically merge into)

Thanks for the great tool! The cli makes my life so much easier, so just hoping to contribute to this awesome project!

@anitameh
Copy link

Hey @ozzyonfire!

On this, the relative URL we recommend using for local testing should help, documented here: https://shopify.dev/docs/apps/build/webhooks/subscribe/https#requirements
image

Recently, we got the ability to configure webhook subsciprions in the app.toml file. However, these uris are not updated on dev (only deployment). This means we need to deploy the app to get proper uris (for testing) which isn't the greatest DX.

I think maybe we can include this information elsewhere so it's obvious, for example in the getting started tutorial with HTTPS delivery. But perhaps there are other places we can surface this as well? Curious to get your thoughts.

@DannyBoris
Copy link

Hey @ozzyonfire!

On this, the relative URL we recommend using for local testing should help, documented here: https://shopify.dev/docs/apps/build/webhooks/subscribe/https#requirements image

Recently, we got the ability to configure webhook subsciprions in the app.toml file. However, these uris are not updated on dev (only deployment). This means we need to deploy the app to get proper uris (for testing) which isn't the greatest DX.

I think maybe we can include this information elsewhere so it's obvious, for example in the getting started tutorial with HTTPS delivery. But perhaps there are other places we can surface this as well? Curious to get your thoughts.


Hey!
I tried it but getting an error on shopify app deploy

  App configuration is not valid             │
│  Validation errors in shopify.app.toml:     │
│                                             │
│  • [webhooks.subscriptions.0.uri]: URI      │
│  isn't correct URI format of https://,      │
│  pubsub://{project}:topic or Eventbridge    │
│  ARN   

@anitameh

@ozzyonfire
Copy link
Author

ozzyonfire commented Jul 29, 2024

Hey @ozzyonfire!

On this, the relative URL we recommend using for local testing should help, documented here: https://shopify.dev/docs/apps/build/webhooks/subscribe/https#requirements image

Recently, we got the ability to configure webhook subsciprions in the app.toml file. However, these uris are not updated on dev (only deployment). This means we need to deploy the app to get proper uris (for testing) which isn't the greatest DX.

I think maybe we can include this information elsewhere so it's obvious, for example in the getting started tutorial with HTTPS delivery. But perhaps there are other places we can surface this as well? Curious to get your thoughts.

Thanks for the reply @anitameh.

I believe the issue is that the relative url (/webhooks) is not getting updated by the cli with the new cloudflare tunnel (or ngrok) domain.

It actually only updates the urls when doing shopify app deploy and not on shopify app dev

That was the issue that I was trying to fix with this PR.

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.

@ozzyonfire
Copy link
Author

Still relevant

@bowenng
Copy link

bowenng commented Sep 19, 2024

Bumping up this PR. This is a very helpful change. Running deploy during development is counter-intuitive and error prone.

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.

@ozzyonfire
Copy link
Author

Still relevant

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.

7 participants