Migrate fullpage redirect to App Bridge CDN #1870
Conversation
kirillplatonov
force-pushed
the
migrate-fullpage-redirect
branch
2 times, most recently
from
71de717
to
e78f74a
Compare
| @@ -1 +1 @@ | |||
| 12.22.8 | |||
There was a problem hiding this comment.
Upgraded as v12 no longer maintained and I couldn't install it locally
kirillplatonov
force-pushed
the
migrate-fullpage-redirect
branch
from
e78f74a
to
ac64247
Compare
kirillplatonov
force-pushed
the
migrate-fullpage-redirect
branch
from
ac64247
to
a2db4cd
Compare
|
@matteodepalo please take a look at this one too. It's important upgrade to move forward with migration to id_token. |
|
Hi @kirillplatonov, we're taking a look now. |
| var appBridgeUtils = window['app-bridge']['utilities']; | ||
|
|
||
| if (appBridgeUtils.isShopifyEmbedded()) { | ||
| if (window['shopify']) { |
There was a problem hiding this comment.
In what situation do we need the else here?
There was a problem hiding this comment.
Removed it as open should cover both embedded and non-embedded context now (great design decision)
| var normalizedLink = document.createElement('a'); | ||
| normalizedLink.href = url; | ||
|
|
||
| Redirect.create(app).dispatch(Redirect.Action.REMOTE, normalizedLink.href); | ||
| open(normalizedLink.href, '_top'); |
| sinon.stub(Redirect, 'create').callsFake(() => RedirectInstance); | ||
|
|
||
| const normalizedUrl = `${window.location.origin}${url}`; | ||
| test('calls App Bridge redirect to normalized url', () => { |
There was a problem hiding this comment.
Not sure this test name is quite true? It's calling window.open? Do we need two tests here? One to test that the App Bridge script tag is in the page and one to test we called window.open?
There was a problem hiding this comment.
Merged and renamed tests. To be honest, I don't like that kind of tests where everything is stabbed and no real behavior being tested. But not sure if there's a better way to test App Bridge. It's a common issue for testing app related code that works with App Bridge too.
| @@ -1,21 +1,9 @@ | |||
| //= require ./app_bridge_3.7.8.js | |||
|
|
|||
| (function(window) { | |||
| function appBridgeRedirect(url) { | |||
There was a problem hiding this comment.
From what I can see, this function is only used once. Instead of adding this function to the window object, could we just called open(normalizedLink.href, '_top') directly?
There was a problem hiding this comment.
Agree, simplified and removed unnecessary function 👍
|
@byrichardpowell made adjustments based on your feedback. Code become even simpler 👍 |
kirillplatonov
force-pushed
the
migrate-fullpage-redirect
branch
from
4cebd6c
to
0de245e
Compare
kirillplatonov
force-pushed
the
migrate-fullpage-redirect
branch
from
0de245e
to
c44d867
Compare
What this PR does
Switching from hardcoded AppBridge 3 version to AppBridge CDN. This removes the need to pass
params[:host]forfullpage_redirect_tohelper and fixes redirect with token exchange auth. I manually tested it with both token exchange and legacy auth.Checklist
Before submitting the PR, please consider if any of the following are needed:
CHANGELOG.mdif the changes would impact usersREADME.md, if appropriate./docs, if necessary