add version parameter to AMP.extension signature

ampproject · Nov 2, 2016 · b9b372d · b9b372d
1 parent e9c309c
commit b9b372d
Show file tree

Hide file tree

Showing 8 changed files with 24 additions and 12 deletions.
diff --git a/build-system/runner/dist/runner.jar b/build-system/runner/dist/runner.jar
diff --git a/build-system/runner/src/org/ampproject/AmpPass.java b/build-system/runner/src/org/ampproject/AmpPass.java
@@ -48,6 +48,8 @@ class AmpPass extends AbstractPostOrderCallback implements HotSwapCompilerPass {
   private final Map<String, Node> prodAssignmentReplacements;
   final boolean isProd;
 
+  final Integer ampExtensionCallbackPosition = 4;
+
   public AmpPass(AbstractCompiler compiler, boolean isProd,
         Map<String, Set<String>> stripTypeSuffixes,
         Map<String, Node> assignmentReplacements, Map<String, Node> prodAssignmentReplacements) {
@@ -99,12 +101,12 @@ public AmpPass(AbstractCompiler compiler, boolean isProd,
    *         NAME self 3 [length: 4] [source_file: input0]
    *         STRING someproperty 3 [length: 15] [source_file: input0]
    *       STRING AMP 3 [length: 3] [source_file: input0]
-   *     STRING etension 3 [length: 12] [source_file: input0]
+   *     STRING extension 3 [length: 12] [source_file: input0]
    *   STRING some-string 3 [length: 9] [source_file: input0]
    *   FUNCTION  3 [length: 46] [source_file: input0]
    */
   private boolean isAmpExtensionCall(Node n) {
-    if (n != null && n.isCall() && n.getChildCount() == 3) {
+    if (n != null && n.isCall() && n.getChildCount() == ampExtensionCallbackPosition) {
       Node getprop = n.getFirstChild();
 
       // The AST has the last getprop higher in the hierarchy.
@@ -115,7 +117,7 @@ private boolean isAmpExtensionCall(Node n) {
                firstChild.getString() == "AMP") ||
             isGetPropName(firstChild, "AMP")) {
           // Child at index 1 should be the "string" value (first argument)
-          Node func = n.getChildAtIndex(2);
+          Node func = getAmpExtensionCallback(n);
           return func != null && func.isFunction();
         }
       }
@@ -136,7 +138,7 @@ private boolean isGetPropName(Node n, String name) {
    * This operation should be guarded stringently by `isAmpExtensionCall`
    * predicate.
    *
-   * AMP.extension('some-name', function(AMP) {
+   * AMP.extension('some-name', '0.1', function(AMP) {
    *   // BODY...
    * });
    *
@@ -149,7 +151,7 @@ private void inlineAmpExtensionCall(Node n, Node expr) {
     if (expr == null || !expr.isExprResult()) {
       return;
     }
-    Node func = n.getChildAtIndex(2);
+    Node func = getAmpExtensionCallback(n);
     func.detachFromParent();
     Node arg1 = IR.getprop(IR.name("self"), IR.string("AMP"));
     arg1.setLength("self.AMP".length());
@@ -161,6 +163,11 @@ private void inlineAmpExtensionCall(Node n, Node expr) {
     compiler.reportCodeChange();
   }
 
+  private Node getAmpExtensionCallback(Node n) {
+    // 0 index based
+    return n.getChildAtIndex(ampExtensionCallbackPosition - 1);
+  }
+
   /**
    * For a function that looks like:
    * function fun(val) {

diff --git a/build-system/runner/test/org/ampproject/AmpPassTest.java b/build-system/runner/test/org/ampproject/AmpPassTest.java
@@ -324,7 +324,7 @@ public void testRemoveAmpAddExtensionCallWithExplicitContext() throws Exception
     testEs6(
         LINE_JOINER.join(
             "var a = 'hello';",
-            "self.AMP.extension('hello', function(AMP) {",
+            "self.AMP.extension('hello', '0.1', function(AMP) {",
             "  var a = 'world';",
             "  console.log(a);",
             "});",
@@ -342,7 +342,7 @@ public void testRemoveAmpAddExtensionCallWithNoContext() throws Exception {
     testEs6(
         LINE_JOINER.join(
             "var a = 'hello';",
-            "AMP.extension('hello', function(AMP) {",
+            "AMP.extension('hello', '0.1', function(AMP) {",
             "  var a = 'world';",
             "  console.log(a);",
             "});",

diff --git a/extensions/amp-sticky-ad/0.1/amp-sticky-ad.js b/extensions/amp-sticky-ad/0.1/amp-sticky-ad.js
@@ -238,6 +238,6 @@ class AmpStickyAd extends AMP.BaseElement {
   }
 }
 
-AMP.extension('amp-sticky-ad:0.1', AMP => {
+AMP.extension('amp-sticky-ad', '0.1', AMP => {
   AMP.registerElement('amp-sticky-ad', AmpStickyAd, CSS);
 });
diff --git a/extensions/amp-sticky-ad/1.0/amp-sticky-ad.js b/extensions/amp-sticky-ad/1.0/amp-sticky-ad.js
@@ -222,6 +222,6 @@ class AmpStickyAd extends AMP.BaseElement {
   }
 }
 
-AMP.extension('amp-sticky-ad:1.0', AMP => {
+AMP.extension('amp-sticky-ad', '1.0', AMP => {
   AMP.registerElement('amp-sticky-ad', AmpStickyAd, CSS);
 });
diff --git a/src/runtime.js b/src/runtime.js
@@ -179,10 +179,11 @@ function adoptShared(global, opts, callback) {
   if (!global.AMP.extension) {
     /**
      * @param {string} unusedName
+     * @param {string} version
      * @param {function(!Object)} installer
      * @const
      */
-    global.AMP.extension = function(unusedName, installer) {
+    global.AMP.extension = function(unusedName, version, installer) {
       installer(global.AMP);
     };
   }

diff --git a/test/_init_tests.js b/test/_init_tests.js
@@ -44,11 +44,12 @@ adopt(window);
 // Override AMP.extension to buffer extension installers.
 /**
  * @param {string} name
+ * @param {string} version
  * @param {function(!Object)} installer
  * @const
  */
-global.AMP.extension = function(name, installer) {
-  describes.bufferExtension(name, installer);
+global.AMP.extension = function(name, version, installer) {
+  describes.bufferExtension(`${name}:${version}`, installer);
 };
 
 

diff --git a/testing/describes.js b/testing/describes.js
@@ -429,6 +429,9 @@ class AmpFixture {
     }
     if (spec.extensions) {
       spec.extensions.forEach(extensionId => {
+        if (extensionId.indexOf(':') != -1) {
+            extensionId = `${extensionId}:0.1`
+        }
         const installer = extensionsBuffer[extensionId];
         if (installer) {
           installer(win.AMP);