-
Notifications
You must be signed in to change notification settings - Fork 478
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 ShopifyAPI::Context.host
to allow for localhost support
#1017
Conversation
d365034
to
1b08398
Compare
a5317e1
to
548736a
Compare
326b800
to
f5c7dcb
Compare
f5c7dcb
to
4697ff5
Compare
ShopifyAPI::Context.host
to allow for localhost support
ShopifyAPI::Context.host
to allow for localhost supportShopifyAPI::Context.host
to allow for localhost support
ShopifyAPI::Context.host
to allow for localhost supportShopifyAPI::Context.host
to allow for localhost support
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.
Just nits and questions!
|
||
sig { returns(String) } | ||
def host_scheme | ||
T.must(URI.parse(T.must(host)).scheme) |
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.
Would it make sense to store host
as a URI
to make it easier to handle later?
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 read some advice a while ago which said that primitives should be only on the boundaries of your system, e.g. where you're reading or outputting data, but within the app boundaries you should aim to work with richer types. Can't say I've managed to fully do that, but I like the concept.
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.
Ya great callout, in an earlier version I had exposed the URI object in fact. When building links with that URI
object I found myself needing to compose the host scheme, host, custom port, and path it and it became way too verbose. Since host
has been used to encapsulate all of those ideas it seemed to be a better tradeoff for simplicity.
In this case I'm using the URI
parse method so I didn't need to write regex to be a little more readable.
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.
All looks good in terms of code – I think some of the names could be more precise, but I understand we may locked into some things for backwards compatibility.
@@ -51,6 +52,7 @@ def setup( | |||
is_embedded:, | |||
session_storage:, | |||
logger: Logger.new($stdout), | |||
host: ENV["HOST"] || "https://#{host_name}", |
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.
Hmm, the naming doesn't feel right here, I wouldn't expect host
to include a protocol.
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.
(maybe call it host_with_scheme
?)
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 agree, the naming here isn't as precise as I'd hope. We use host
name to imply the needed scheme. For that consistency I think we should leave it as is :(
This is busted in my life. Here is why. I use Puma Dev to name my localhost. So I have a name and port. It works! So to Shopify App, my App is configured as https://myapp.test and that works fine, as under the hood, this gets converted to an ngrok tunnel and away we go. But you cannot use this host for webhooks. Nope. For webhooks to work you need to provide an actual reachable URI which is https://myapp.ngrok.io Unfortunately, this change with webhooks is now writing all my localhost webhooks as https://myapp.test//webooks/app_uninstalled as an example, which is not reachable. So I am screwed now when my older working pattern is used with this new code. Why is it this way? I dunno. But it is. Is there a way to register a webhook and not have it play havoc with the provided callback URL? In the Shopify App if I use this:
The webhook I find registered in the store is https://myapp.test/webhooks/app_uninstalled which is unreachable. That is because my host is indeed myapp.test. But the reality is, webhooks work perfect when I use a manual setup and set the callback URL to be https://myapp.ngrok.io This is frustrating. |
Description
The default CLI is to use
localhost
for OAuth instead of using a tunnel 🎉 . We have an assumption throughout the codebase that thehttps
scheme is used. In order to getlocalhost
to work with this gem, we need remove that assumption.This adds the following:
ShopifyAPI::Context.host
which will include the scheme, name, and port of the host appShopifyAPI::Context.host_scheme
helper to returnhttp
orhttps
for the few edge cases when we'd need itThe
host
is set by the configuration, ENV variable, or deduced fromhost_name
.This will work with backward compatibility and won't need updated configs to work. We will update shopify_app's generated config to include
host
.How has this been tested?
Please, describe the tests that you ran to verify your changes.
I'll be developing another PR in the shopify_app gem in tandem with this. I'll ensure that I can get localhost as well as tunnel with https to work with this gem.
Checklist: