This repository has been archived by the owner on Apr 11, 2024. It is now read-only.
Add a new utility for constructing the host URL for the app. #419
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
|
@paulomarg @vividviolet this is now ready for review. I'm going to test the code manually in the app template. |
byrichardpowell
commented
Jul 12, 2022
vividviolet
reviewed
Jul 12, 2022
src/utils/get-host-app-url.ts
Outdated
| ); | ||
| } | ||
|
|
||
| if (!request.url) { |
There was a problem hiding this comment.
Yeah, this really surprised me, but according to the type definitions it can be undefined:
Unless I've mis-understood something?
There was a problem hiding this comment.
I thought url was always a string according to this https://nodejs.org/api/http.html#messageurl
byrichardpowell
force-pushed
the
add-util-for-app-url
branch
from
6da23f4
to
774e7a8
Compare
@vividviolet Agreed. |
byrichardpowell
force-pushed
the
add-util-for-app-url
branch
from
774e7a8
to
02bd65c
Compare
byrichardpowell
requested review from
vividviolet,
henrytao-me and
mkevinosullivan
paulomarg
approved these changes
Jul 13, 2022
byrichardpowell
force-pushed
the
add-util-for-app-url
branch
3 times, most recently
from
05ea6a2
to
ab55983
Compare
1. Correct typo 2. Change utility name "getHostAppUrl" to "getEmbeddedAppUrl", which is much nicer. 3. Add / to the host in our test. This matches the format of the host that Shopify provides 4. Flesh out the docs a little more. 5. Use new URL, rather than url.parse. url.parse is deprecated.
byrichardpowell
force-pushed
the
add-util-for-app-url
branch
from
ab55983
to
4cf0bef
Compare
This was referenced Aug 2, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
WHY are these changes introduced?
For performance soon we will be recommending that apps favor server redirects within the OAuth process. This will replace client side redirects using AppBridge. We want to control that code that constructs the URL for the app. That way it's easier for developers, and if that URL changes we can update the logic with a version bump.
WHAT is this pull request doing?
Add a new utility called
getHostAppUrl. This utility constructs the host URL for an app.This utility will be used to get the URL to redirect to when the store does not have a token for the app.
Type of change
Checklist