Conversation
5c32da5 to
96a5a2f
Compare
96a5a2f to
f3e6f44
Compare
c1fd2ae to
f34485f
Compare
f34485f to
2216206
Compare
| } else { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
This was just moved below for consistent ordering.
| * @return {T} | ||
| */ | ||
| export function getService(win, id) { | ||
| win = getTopWindow(win); |
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
| 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
| 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.
6f5686a to
9da9cd8
Compare
|
|
||
| /** | ||
| 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.
| 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.