Fix profiling for stores with custom domains #136
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dan-menard
approved these changes
Jan 24, 2024
| @@ -84,7 +84,7 @@ function clear() { | |||
| function getInspectedWindowURL(): Promise<URL> { | |||
| return new Promise(resolve => { | |||
| chrome.devtools.inspectedWindow.eval( | |||
| `(/myshopify\\.io/.test(document.location.host) ? document.location.host : Shopify.shop) + document.location.pathname + document.location.search`, | |||
| `document.location.host + document.location.pathname + document.location.search`, | |||
There was a problem hiding this comment.
What was this code supposed to do, originally?
I think the old version is actually broken:
So it would have consistently used Shopify.shop to build the URL.
The fix makes sense to me, because it's simpler and will work in more cases. I'm mostly curious if anything is accidentally relying on Shopify.shop.
There was a problem hiding this comment.
I believe it comes from here. However, my understanding is that document.location.host should handle both cases (localhost and non-localhost).
karreiro
approved these changes
Jan 24, 2024
|
@wizardlyhel would it be possible to get this reviewed and merged please? :-) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What issue does this pull request address?
/* Context about the problem such as a link to an issue or just a description */
When profiling any Shopify store with custom domain, the run fails with
This page cannot be profiledmessage. After some investigating, I noticed that the extension is always doing the?profile_liquid=truerequest to the permanent.myshopify.comdomain instead of the custom one:As you can see (probably because of some recent configuration changes), this results in the 301 redirect to the custom domain. The problem is that this makes the request lose the
Authorizationheader and so it fails.What is the solution
/* Describe the solutions effectively */
I'm not sure what was the original reason for the conditional, but it seems like we can just always use the
document.location.host. I haven't set the extension locally, but overwriting the value ofShopify.shopto be the custom domain fixes the issue and lets me profile the store properly.What should the reviewer focus on and are there any special considerations?
/* Any additional details you would want to bring to attention */