-
Notifications
You must be signed in to change notification settings - Fork 35
OAuth and webhooks when using Shopify CLI #81
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
base: v3
Are you sure you want to change the base?
Conversation
lib/shopifex/oauth.ex
Outdated
| # https://shopify.dev/docs/apps/build/authentication-authorization/access-tokens/authorization-code-grant#check-for-and-escape-the-iframe-embedded-apps-only | ||
| if Map.get(conn.params, "embedded") == "1" do | ||
| conn | ||
| |> Phoenix.Controller.put_layout(html: {ShopifexWeb.LayoutView, :app}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using phoenix 1.7 syntax here as my app was complaining about not using this. Not sure how todo backwards compat here.
|
|
||
| def current_shopify_host(_), do: nil | ||
| def current_shopify_host(conn) do | ||
| Map.get(conn.params, "host") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will allow the app bridge to render during install redirect as host is required
| nil -> | ||
| redirect_to_install(conn, shop_url) | ||
| conn | ||
| |> redirect(to: "/initialize-installation?#{conn.query_string}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoding this redirect doesn't feel quite right
|
Looks really interesting at first glance. I will test it out in my own Shopify apps to see if there are any compatibility issues coming up. What was the rest of your setup in regards to using Shopify CLI alongside Shopifex? |
My folder structure basically is as seen here https://github.com/NexPB/phx-shopify-app-example
Afterwards I start the my local env using Hope that answers your question. |
|
That's really interesting! So you have your dev server running with I will review this more thoroughly soon, but this will definitely justify an update to the readme for this library. |
Yeah makes things pretty seamless
I'll try adding that to the PR this weekend |
|
@ericdude4 I've added a section for the Shopify CLI at the end of the README |
ericdude4
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good. A few nit picky comments. I tried to review as thoroughly as possible without having used the CLI before. I really appreciate this contribution!!! I look forward to testing this out.
| ## Options | ||
| - `:cli` - If true, the callback route will be `/auth/callback` instead of `/auth/install`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing this option again I wonder if we can make this backwards compatible without needing an option at all 🤔
Since the install and update are pretty much the same as the callback apart from the different after_* callbacks.
Maybe just:
get "/install", unquote(controller), :callback
get "/update", unquote(controller), :callbackThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is /auth/callback only used during development?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is /auth/callback only used during development?
I've replied below, but tldr it's used for all envs when using the CLI.
| - `:cli` - If true, the callback route will be `/auth/callback` instead of `/auth/install`. | ||
| """ | ||
| defmacro auth_routes(controller \\ ShopifexWeb.AuthController, opts \\ []) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could include this specification for ShopifexWeb.AuthController in the shopifex config. Then, it would simplify the usage of this macro in the router and we wouldn't have to hardcode the /initialize-installation route on https://github.com/ericdude4/shopifex/pull/81/files#r2003145269?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean something like config :shopifex, cli: true ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about something like config :shopifex, auth_controller: ShopifexWeb.AuthController. Then it could be overridden with ShopifexWeb.Routes.auth_routes(auth_controller: FooWeb.AuthController) if that's needed for some reason. This approach seems in line with what i see in other libraries.
Might be worthwhile to introduce a Shopifex.Config module which takes care of loading all the config and managing defaults, etc.
With this defined in a global config, then it would be easier to use something like a RouteHelper to generate the /initialize-installation route which is currently being harcoded.
| scope "/auth" do | ||
| pipe_through [:shopifex_browser, :validate_install_hmac] | ||
|
|
||
| get "/callback", unquote(controller), :callback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious about the difference between /auth/callback and /auth/install/update. Is one used during development by the Shopify CLI and the other is used in production?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the CLI only /auth/callback is required generally only one route is used for both install and update. Hence those routes not really being used when using the CLI, thus being omited by the cli option but it does "complicate" the setup of shopifex as it would be easy to forget to add the option.
|
There are a few places in the codebase where we mention deprecating things when updating to version 3.x. I wonder if we take this opportunity to release a new 3.x version of shopifex as a release candidate. I happen to be just starting to build a new app with Shopifex, so it would be a great way to test too. |
I did notice some changes that would benefit a 3.x release such as using some newer http libraries (I'm quite fond of |
@NexPB I think a 3.x release is a fantastic idea. I really appreciate your willingness to help with this! I wonder if we create a Do you have discord? Maybe we can continue the conversation there? |
Yeah, my discord username is |
|
Following these changes. @NexPB your sample repo is quite helpful! Has anyone been using things with the Phoenix 1.8 RCs? |
Have only been using this with 1.7 |
|
What's the state of this? Will it get merged? |
These days Shopify is pushing the CLI hard esp. when you follow their dev guides. These changes allow you get get going with the CLI more quickly when using
shopifex. (PR is in draft as I would like to get some feedback)Key changes
/auth/callback(when usingautomatically_update_urls_on_dev)I had trouble with the app not rendering (upon install) after the oauth flow had finished, I assume because it was embedded and required the app bridge redirect flow so I've made changes to account for that.