Fix amp-bind in PWA #10131
Fix amp-bind in PWA #10131
Conversation
dreamofabear
force-pushed
the
bind-in-pwa
branch
from
5c32da5
to
96a5a2f
Compare
dreamofabear
force-pushed
the
bind-in-pwa
branch
from
96a5a2f
to
f3e6f44
Compare
dreamofabear
force-pushed
the
bind-in-pwa
branch
from
c1fd2ae
to
f34485f
Compare
dreamofabear
force-pushed
the
bind-in-pwa
branch
from
f34485f
to
2216206
Compare
| } else { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
This was just moved below for consistent ordering.
| */ | ||
| export function getService(win, id) { | ||
| win = getTopWindow(win); | ||
| return getServiceInternal(win, id); |
There was a problem hiding this comment.
This was just moved below for consistent ordering.
|
/to @jridgewell PTAL 👀 |
extensions/amp-bind/0.1/bind-impl.js
Outdated
| @@ -135,15 +136,21 @@ export class Bind { | |||
| /** @const @private {!../../../src/service/viewer-impl.Viewer} */ | |||
| this.viewer_ = viewerForDoc(this.ampdoc); | |||
|
|
|||
| /** | |||
| const bodyReadyPromise = (opt_win) | |||
There was a problem hiding this comment.
Doesn't this resolve to a body?
There was a problem hiding this comment.
whenBodyAvailable does, but waitForBodyPromise doesn't. Changed it to use resolve value but not sure it's cleaner.
src/element-service.js
Outdated
| const win = /** @type {!Document} */ ( | ||
| nodeOrDoc.ownerDocument || nodeOrDoc).defaultView; | ||
| return elementServicePromiseOrNull(win, id, extension); | ||
| const topWin = getTopWindow(win); | ||
| // Don't return promise unless this is definitely FIE to avoid covfefe. |
There was a problem hiding this comment.
I don't understand this.
There was a problem hiding this comment.
Made this comment clearer. The current system is pretty confusing and I'm planning on refactoring this soon.
dreamofabear
force-pushed
the
bind-in-pwa
branch
from
6f5686a
to
9da9cd8
Compare
| @@ -135,17 +136,19 @@ export class Bind { | |||
| /** @const @private {!../../../src/service/viewer-impl.Viewer} */ | |||
| this.viewer_ = viewerForDoc(this.ampdoc); | |||
|
|
|||
| /** | |||
| const bodyPromise = (opt_win) | |||
| ? waitForBodyPromise(opt_win.document) | |||
There was a problem hiding this comment.
You should make it resolve to a body. 😉
There was a problem hiding this comment.
😩 How about in fix-it next week?
extensions/amp-bind/0.1/bind-impl.js
Outdated
| return this.initialize_(rootNode); | ||
| }); | ||
| this.initializePromise_ = | ||
| this.viewer_.whenFirstVisible().then(bodyPromise).then(body => { |
There was a problem hiding this comment.
the then(bodyPromise) does not return a Promise, it returns whenFirstVisible's again. You have to pass in a function.
| if (nodeOrDoc.nodeType) { | ||
| const win = /** @type {!Document} */ ( | ||
| nodeOrDoc.ownerDocument || nodeOrDoc).defaultView; | ||
| return elementServicePromiseOrNull(win, id, extension); | ||
| const topWin = getTopWindow(win); | ||
| // In embeds, doc-scope services are window-scope. But make sure to |
There was a problem hiding this comment.
I still don't understand it, but ok.
Fixes #9916.
ampdoc.getBody(), notampdoc.win.document.body(doesn't work for shadow docs)element-service.jsandbind-impl.jsThis PR also requires passing a boolean
opt_fallbackToTopWinto allow crossing FIE boundary when getting services. I think this is a safer default, e.g. for A4A.