-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix amp-bind in PWA #10131
Fix amp-bind in PWA #10131
Changes from all commits
ec7bd11
5007ef9
348b0de
f3e6f44
0c5129e
2216206
9da9cd8
6ca2f51
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ import { | |
getAmpdoc, | ||
getServicePromiseForDoc, | ||
getServicePromiseOrNullForDoc, | ||
getTopWindow, | ||
} from './service'; | ||
import {user} from './log'; | ||
import * as dom from './dom'; | ||
|
@@ -61,7 +62,7 @@ export function getElementServiceIfAvailable(win, id, extension, opt_element) { | |
if (s) { | ||
return /** @type {!Promise<?Object>} */ (s); | ||
} | ||
return elementServicePromiseOrNull(win, id, extension, opt_element); | ||
return getElementServicePromiseOrNull(win, id, extension, opt_element); | ||
} | ||
|
||
/** | ||
|
@@ -173,10 +174,17 @@ export function getElementServiceIfAvailableForDocInEmbedScope( | |
if (s) { | ||
return /** @type {!Promise<?Object>} */ (Promise.resolve(s)); | ||
} | ||
// Return embed-scope element service promise if scheduled. | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still don't understand it, but ok. |
||
// only do this for embeds (not the top window), otherwise we'd grab | ||
// a promise from the wrong service holder which would never resolve. | ||
if (win !== topWin) { | ||
return getElementServicePromiseOrNull(win, id, extension); | ||
} | ||
} | ||
return /** @type {!Promise<?Object>} */ (Promise.resolve(null)); | ||
} | ||
|
@@ -207,7 +215,7 @@ function assertService(service, id, extension) { | |
* @return {!Promise<Object>} | ||
* @private | ||
*/ | ||
function elementServicePromiseOrNull(win, id, extension, opt_element) { | ||
function getElementServicePromiseOrNull(win, id, extension, opt_element) { | ||
// Microtask is necessary to ensure that window.ampExtendedElements has been | ||
// initialized. | ||
return Promise.resolve().then(() => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,12 @@ | |
* limitations under the License. | ||
*/ | ||
|
||
/** | ||
* @fileoverview Registration and getter functions for AMP services. | ||
* | ||
* Invariant: Service getters never return null for registered services. | ||
*/ | ||
|
||
// Requires polyfills in immediate side effect. | ||
import './polyfills'; | ||
import {dev} from './log'; | ||
|
@@ -68,46 +74,37 @@ export class EmbeddableService { | |
adoptEmbedWindow(unusedEmbedWin) {} | ||
} | ||
|
||
/** | ||
* Returns a service or null with the given id. | ||
* @param {!Window} win | ||
* @param {string} id | ||
* @return {?Object} The service. | ||
*/ | ||
export function getExistingServiceOrNull(win, id) { | ||
win = getTopWindow(win); | ||
if (isServiceRegistered(win, id)) { | ||
return getServiceInternal(win, id); | ||
} else { | ||
return null; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was just moved below for consistent ordering. |
||
|
||
/** | ||
* Returns a service with the given id. Assumes that it has been registered | ||
* already. | ||
* @param {!Window} win | ||
* @param {string} id | ||
* @return {!Object} The service. | ||
* @param {boolean=} opt_fallbackToTopWin | ||
* @return {Object} The service. | ||
*/ | ||
export function getExistingServiceInEmbedScope(win, id) { | ||
export function getExistingServiceInEmbedScope(win, id, opt_fallbackToTopWin) { | ||
// First, try to resolve via local (embed) window. | ||
const local = getLocalExistingServiceForEmbedWinOrNull(win, id); | ||
if (local) { | ||
return local; | ||
} | ||
// Fallback to top-level window. | ||
return getService(win, id); | ||
if (opt_fallbackToTopWin) { | ||
return getService(win, id); | ||
} | ||
return null; | ||
} | ||
|
||
/** | ||
* Returns a service with the given id. Assumes that it has been constructed | ||
* already. | ||
* @param {!Node|!./service/ampdoc-impl.AmpDoc} nodeOrDoc | ||
* @param {string} id | ||
* @return {!Object} The service. | ||
* @param {boolean=} opt_fallbackToTopWin | ||
* @return {Object} The service. | ||
*/ | ||
export function getExistingServiceForDocInEmbedScope(nodeOrDoc, id) { | ||
export function getExistingServiceForDocInEmbedScope( | ||
nodeOrDoc, id, opt_fallbackToTopWin) { | ||
// First, try to resolve via local (embed) window. | ||
if (nodeOrDoc.nodeType) { | ||
// If a node is passed, try to resolve via this node. | ||
|
@@ -118,8 +115,11 @@ export function getExistingServiceForDocInEmbedScope(nodeOrDoc, id) { | |
return local; | ||
} | ||
} | ||
// Fallback to ampdoc. | ||
return getServiceForDoc(nodeOrDoc, id); | ||
// If an ampdoc is passed or fallback is allowed, continue resolving. | ||
if (!nodeOrDoc.nodeType || opt_fallbackToTopWin) { | ||
return getServiceForDoc(nodeOrDoc, id); | ||
} | ||
return null; | ||
} | ||
|
||
/** | ||
|
@@ -134,13 +134,8 @@ export function installServiceInEmbedScope(embedWin, id, service) { | |
'Service override can only be installed in embed window: %s', id); | ||
dev().assert(!getLocalExistingServiceForEmbedWinOrNull(embedWin, id), | ||
'Service override has already been installed: %s', id); | ||
registerServiceInternal( | ||
embedWin, | ||
embedWin, | ||
id, | ||
() => service); | ||
// Force service to build | ||
getServiceInternal(embedWin, id); | ||
registerServiceInternal(embedWin, embedWin, id, () => service); | ||
getServiceInternal(embedWin, id); // Force service to build. | ||
} | ||
|
||
/** | ||
|
@@ -160,21 +155,6 @@ function getLocalExistingServiceForEmbedWinOrNull(embedWin, id) { | |
} | ||
} | ||
|
||
/** | ||
* Returns a service for the given id and window (a per-window singleton). | ||
* Users should typically wrap this as a special purpose function (e.g. | ||
* `vsyncFor(win)`) for type safety and because the factory should not be | ||
* passed around. | ||
* @param {!Window} win | ||
* @param {string} id of the service. | ||
* @template T | ||
* @return {T} | ||
*/ | ||
export function getService(win, id) { | ||
win = getTopWindow(win); | ||
return getServiceInternal(win, id); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was just moved below for consistent ordering. |
||
} | ||
|
||
/** | ||
* Registers a service given a class to be used as implementation. | ||
* @param {!Window} win | ||
|
@@ -193,6 +173,7 @@ export function registerServiceBuilder(win, | |
} | ||
} | ||
|
||
|
||
/** | ||
* Returns a service and registers it given a class to be used as | ||
* implementation. | ||
|
@@ -213,6 +194,23 @@ export function registerServiceBuilderForDoc(nodeOrDoc, | |
} | ||
} | ||
|
||
|
||
/** | ||
* Returns a service for the given id and window (a per-window singleton). | ||
* Users should typically wrap this as a special purpose function (e.g. | ||
* `vsyncFor(win)`) for type safety and because the factory should not be | ||
* passed around. | ||
* @param {!Window} win | ||
* @param {string} id of the service. | ||
* @template T | ||
* @return {T} | ||
*/ | ||
export function getService(win, id) { | ||
win = getTopWindow(win); | ||
return getServiceInternal(win, id); | ||
} | ||
|
||
|
||
/** | ||
* Returns a promise for a service for the given id and window. Also expects | ||
* an element that has the actual implementation. The promise resolves when | ||
|
@@ -229,6 +227,22 @@ export function getServicePromise(win, id) { | |
} | ||
|
||
|
||
/** | ||
* Returns a service or null with the given id. | ||
* @param {!Window} win | ||
* @param {string} id | ||
* @return {?Object} The service. | ||
*/ | ||
export function getExistingServiceOrNull(win, id) { | ||
win = getTopWindow(win); | ||
if (isServiceRegistered(win, id)) { | ||
return getServiceInternal(win, id); | ||
} else { | ||
return null; | ||
} | ||
} | ||
|
||
|
||
/** | ||
* Like getServicePromise but returns null if the service was never registered. | ||
* @param {!Window} win | ||
|
@@ -239,6 +253,7 @@ export function getServicePromiseOrNull(win, id) { | |
return getServicePromiseOrNullInternal(win, id); | ||
} | ||
|
||
|
||
/** | ||
* Returns a service for the given id and ampdoc (a per-ampdoc singleton). | ||
* Expects service `id` to be registered. | ||
|
@@ -253,6 +268,7 @@ export function getServiceForDoc(nodeOrDoc, id) { | |
return getServiceInternal(holder, id); | ||
} | ||
|
||
|
||
/** | ||
* Returns a promise for a service for the given id and ampdoc. Also expects | ||
* a service that has the actual implementation. The promise resolves when | ||
|
@@ -263,8 +279,7 @@ export function getServiceForDoc(nodeOrDoc, id) { | |
*/ | ||
export function getServicePromiseForDoc(nodeOrDoc, id) { | ||
return getServicePromiseInternal( | ||
getAmpdocServiceHolder(nodeOrDoc), | ||
id); | ||
getAmpdocServiceHolder(nodeOrDoc), id); | ||
} | ||
|
||
|
||
|
@@ -277,8 +292,7 @@ export function getServicePromiseForDoc(nodeOrDoc, id) { | |
*/ | ||
export function getServicePromiseOrNullForDoc(nodeOrDoc, id) { | ||
return getServicePromiseOrNullInternal( | ||
getAmpdocServiceHolder(nodeOrDoc), | ||
id); | ||
getAmpdocServiceHolder(nodeOrDoc), id); | ||
} | ||
|
||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should make it resolve to a
body
. 😉There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😩 How about in fix-it next week?