🐛 Forbid private properties inside BaseElement

.eslintrc.js

@@ -351,5 +351,11 @@ module.exports = {
         'google-camelcase/google-camelcase': 0,
       },
     },
+    {
+      'files': ['src/base-element.js'],
+      'rules': {
+        'local/no-private-props': 2,
+      },
+    },
   ],
 };

build-system/eslint-rules/no-private-props.js

@@ -0,0 +1,66 @@
+/**
+ * Copyright 2016 The AMP HTML Authors. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS-IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+'use strict';
+
+/**
+ * Ensures private properties are not used in the file. If they are, they must
+ * be quoted.
+ *
+ * @return {!Object}
+ */
+module.exports = {
+  meta: {
+    fixable: 'code',
+  },
+
+  create(context) {
+    return {
+      MemberExpression(node) {
+        if (node.computed || !node.property.name.endsWith('_')) {
+          return;
+        }
+
+        context.report({
+          node,
+          message:
+            'Unqouted private properties are not allowed in BaseElement. Please use quotes',
+          fix(fixer) {
+            const {object} = node;
+            return fixer.replaceTextRange(
+              [object.end, node.end],
+              `['${node.property.name}']`
+            );
+          },
+        });
+      },
+
+      MethodDefinition(node) {
+        if (node.computed || !node.key.name.endsWith('_')) {
+          return;
+        }
+
+        context.report({
+          node,
+          message:
+            'Unqouted private methods are not allowed in BaseElement. Please use quotes',
+          fix(fixer) {
+            return fixer.replaceText(node.key, `['${node.key.name}']`);
+          },
+        });
+      },
+    };
+  },
+};

build-system/eslint-rules/private-prop-names.js

@@ -55,7 +55,7 @@ module.exports = function (context) {
     MethodDefinition: function (node) {
       if (
         hasPrivateAnnotation(context.getCommentsBefore(node)) &&
-        !hasTrailingUnderscore(node.key.name)
+        !hasTrailingUnderscore(node.key.name || node.key.value)
       ) {
         context.report({
           node,
@@ -68,7 +68,9 @@ module.exports = function (context) {
         node.parent.type == 'ExpressionStatement' &&
         hasPrivateAnnotation(context.getCommentsBefore(node.parent)) &&
         isThisMemberExpression(node.left) &&
-        !hasTrailingUnderscore(node.left.property.name)
+        !hasTrailingUnderscore(
+          node.left.property.name || node.left.property.value
+        )
       ) {
         context.report({
           node,

src/base-element.js

@@ -229,34 +229,34 @@ export class BaseElement {
   constructor(element) {
     /** @public @const {!Element} */
     this.element = element;
+
+    /** @public @const {!Window} */
+    this.win = toWin(element.ownerDocument.defaultView);
+
     /*
     \   \  /  \  /   / /   \     |   _  \     |  \ |  | |  | |  \ |  |  /  ____|
      \   \/    \/   / /  ^  \    |  |_)  |    |   \|  | |  | |   \|  | |  |  __
       \            / /  /_\  \   |      /     |  . `  | |  | |  . `  | |  | |_ |
        \    /\    / /  _____  \  |  |\  \----.|  |\   | |  | |  |\   | |  |__| |
         \__/  \__/ /__/     \__\ | _| `._____||__| \__| |__| |__| \__|  \______|
 
-    Any private property for BaseElement should be declared in
-    build-system/externs/amp.multipass.extern.js. This is so closure compiler
-    doesn't reuse the same symbol it would use in the core compilation unit for
-    the private property in the extensions compilation unit's private
-    properties.
+    Any private property for BaseElement MUST be wrapped with quotes. We cannot
+    allow Closure Compiler to mangle privates in this class, becasue it can
+    reuse the same mangled name for a different property in, ie., amp-youtube's
+    BaseElement subclass (which lives in a different binary).
     */
 
-    /** @public @const {!Window} */
-    this.win = toWin(element.ownerDocument.defaultView);
-
     /**
      * Maps action name to struct containing the action handler and minimum
      * trust required to invoke the handler.
      * @private {?Object<string, {
      *   handler: function(!./service/action-impl.ActionInvocation),
      *   minTrust: ActionTrust,
      * }>} */
-    this.actionMap_ = null;
+    this['actionMap_'] = null;
 
     /** @private {?string} */
-    this.defaultActionAlias_ = null;
+    this['defaultActionAlias_'] = null;
   }
 
   /**
@@ -272,7 +272,7 @@ export class BaseElement {
    * @return {?string}
    */
   getDefaultActionAlias() {
-    return this.defaultActionAlias_;
+    return this['defaultActionAlias_'];
   }
 
   /**
@@ -678,13 +678,6 @@ export class BaseElement {
     return loadPromise(element);
   }
 
-  /** @private */
-  initActionMap_() {
-    if (!this.actionMap_) {
-      this.actionMap_ = this.win.Object.create(null);
-    }
-  }
-
   /**
    * Registers the action handler for the method with the specified name.
    *
@@ -697,8 +690,8 @@ export class BaseElement {
    * @public
    */
   registerAction(alias, handler, minTrust = ActionTrust.DEFAULT) {
-    this.initActionMap_();
-    this.actionMap_[alias] = {handler, minTrust};
+    initActionMap(this);
+    this['actionMap_'][alias] = {handler, minTrust};
   }
 
   /**
@@ -714,12 +707,12 @@ export class BaseElement {
     minTrust = ActionTrust.DEFAULT
   ) {
     devAssert(
-      !this.defaultActionAlias_,
+      !this['defaultActionAlias_'],
       'Default action "%s" already registered.',
-      this.defaultActionAlias_
+      this['defaultActionAlias_']
     );
     this.registerAction(alias, handler, minTrust);
-    this.defaultActionAlias_ = alias;
+    this['defaultActionAlias_'] = alias;
   }
 
   /**
@@ -737,10 +730,10 @@ export class BaseElement {
     let {method} = invocation;
     // If the default action has an alias, the handler will be stored under it.
     if (method === DEFAULT_ACTION) {
-      method = this.defaultActionAlias_ || method;
+      method = this['defaultActionAlias_'] || method;
     }
-    this.initActionMap_();
-    const holder = this.actionMap_[method];
+    initActionMap(this);
+    const holder = this['actionMap_'][method];
     const {tagName} = this.element;
     userAssert(holder, `Method not found: ${method} in ${tagName}`);
     const {handler, minTrust} = holder;
@@ -1121,3 +1114,15 @@ export class BaseElement {
     return this;
   }
 }
+
+/**
+ * This would usually be a private method on BaseElement class, but we cannot
+ * use privates here. So, it's manually devirtualized into a regular function.
+ *
+ * @param {typeof BaseElement} baseElement
+ */
+function initActionMap(baseElement) {
+  if (!baseElement['actionMap_']) {
+    baseElement['actionMap_'] = baseElement.win.Object.create(null);
+  }
+}