Amp action macro cleanup #20841
Amp action macro cleanup #20841
Conversation
alabiaga
commented
Feb 13, 2019
•
alabiaga
commented
- do not allow a macro to recursively call itself.
- add examples page.
…ntime that doc height could have changed
|
@choumx Hey Will note that you referred to the amp-bind-macro and its check for ensuring it refers to macros that were predefined before it. For the amp-action macro this isn't an issue as when we hit the trigger code path in the action macro, it is already guaranteed to be defined. The existence of the target/amp-component that it will invoke is also getting checked in the actions service. |
| isRecursiveInvocation_(invocation) { | ||
| return invocation.source.getAttribute('id') | ||
| === this.element.getAttribute('id') | ||
| && invocation.source.tagName === 'AMP-ACTION-MACRO'; |
There was a problem hiding this comment.
We need to worry about cycles generally. I.e. not just x -> x but x -> y -> x.
|
Gotcha but why not just not let the JS engine max call stack count handle
this? To me this is more of an error on the developer client side.
…On Thu, Feb 14, 2019, 5:34 PM William Chou ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In extensions/amp-action-macro/0.1/amp-action-macro.js
<#20841 (comment)>:
> @@ -87,6 +88,18 @@ export class AmpActionMacro extends AMP.BaseElement {
// We want the macro to be available wherever it is in the document.
return true;
}
+
+ /**
+ * Checks if the action is triggering itself.
+ * @param {!../../../src/service/action-impl.ActionInvocation} invocation
+ * @return {boolean}
+ * @Private
+ */
+ isRecursiveInvocation_(invocation) {
+ return invocation.source.getAttribute('id')
+ === this.element.getAttribute('id')
+ && invocation.source.tagName === 'AMP-ACTION-MACRO';
We need to worry about cycles generally. I.e. not just x -> x but x -> y
-> x.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#20841 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAeKUDEZd0koKFgs0hHMagFkazq6zP5yks5vNeR_gaJpZM4a6X8M>
.
|
| */ | ||
| isRecursiveInvocation_(invocation) { | ||
| return invocation.caller.getAttribute('id') | ||
| === this.element.getAttribute('id'); |
There was a problem hiding this comment.
The other check should cover this case.
| * @private | ||
| */ | ||
| isInvalidMacroReference_(invocationRegistrationId) { | ||
| return invocationRegistrationId <= this.element['registrationId']; |
There was a problem hiding this comment.
Can we use this instead? https://developer.mozilla.org/en-US/docs/Web/API/Node/compareDocumentPosition
There was a problem hiding this comment.
This is new to me. Fancy. done.
| @@ -75,9 +81,16 @@ export class AmpActionMacro extends AMP.BaseElement { | |||
| arg, this.element); | |||
| } | |||
| } | |||
| if (invocation.caller.tagName === 'AMP-ACTION-MACRO') { | |||
There was a problem hiding this comment.
invocation.caller.tagName.toLowerCase() === TAG
| * @return {boolean} | ||
| * @private | ||
| */ | ||
| isInvalidMacroReference_(invocationRegistrationId) { |
There was a problem hiding this comment.
Nit: Single caller of a simple function -- can be inlined.
|
@choumx friendly ping |
| @@ -75,9 +75,13 @@ export class AmpActionMacro extends AMP.BaseElement { | |||
| arg, this.element); | |||
| } | |||
| } | |||
| if (invocation.caller.tagName.toLowerCase() === TAG) { | |||
| userAssert(this.isValidMacroReference_( | |||
| invocation.caller), | |||
| userAssert(this.isValidMacroReference_( | ||
| invocation.caller), | ||
| 'Action macro with ID "%s" cannot reference itself or macros defined ' | ||
| + 'after it', this.element.getAttribute('id')); |
There was a problem hiding this comment.
"amp-action-macro#%s may only reference macros that precede it."
|
thanks! |
* toggle maybeChangeHeight in layers after remeasures to indicate to runtime that doc height could have changed * cleanup * revert resources-impl change * revert this change from master * . * e.g. of invalid call * counter can simply be an incrementing num * choumx comments
* toggle maybeChangeHeight in layers after remeasures to indicate to runtime that doc height could have changed * cleanup * revert resources-impl change * revert this change from master * . * e.g. of invalid call * counter can simply be an incrementing num * choumx comments
