Lint: isExperimentOn must be passed an explicit string

The root cause of #16671 (from #16528) is difficutly grepping for used experiment flags. So, make them very grepable. ```js // BAD const EXP = 'experiment-name'; isExperimentOn(win, EXP); // GOOD isExperimentOn(win, 'experiment-name');
ampproject · Jul 12, 2018 · b1d401e · b1d401e
1 parent 424697f
commit b1d401e
Show file tree

Hide file tree

Showing 24 changed files with 90 additions and 67 deletions.
diff --git a/.eslintrc b/.eslintrc
@@ -62,6 +62,7 @@
     "amphtml-internal/dict-string-keys": 2,
     "amphtml-internal/enforce-private-props": 2,
     "amphtml-internal/html-template": 2,
+    "amphtml-internal/is-experiment-on": 2,
     "amphtml-internal/no-array-destructuring": 2,
     "amphtml-internal/no-deep-destructuring": 2,
     "amphtml-internal/no-es2015-number-props": 2,

diff --git a/ads/google/a4a/utils.js b/ads/google/a4a/utils.js
@@ -149,14 +149,13 @@ export function isReportingEnabled(ampElement) {
   // a no-op (sends no pings).
   const type = ampElement.element.getAttribute('type');
   const {win} = ampElement;
-  const experimentName = 'a4aProfilingRate';
   // In local dev mode, neither the canary nor prod config files is available,
   // so manually set the profiling rate, for testing/dev.
   if (getMode(ampElement.win).localDev && !getMode(ampElement.win).test) {
-    toggleExperiment(win, experimentName, true, true);
+    toggleExperiment(win, 'a4aProfilingRate', true, true);
   }
   return (type == 'doubleclick' || type == 'adsense') &&
-      isExperimentOn(win, experimentName);
+      isExperimentOn(win, 'a4aProfilingRate');
 }
 
 /**

diff --git a/build-system/eslint-rules/is-experiment-on.js b/build-system/eslint-rules/is-experiment-on.js
@@ -0,0 +1,43 @@
+/**
+ * Copyright 2018 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';
+
+module.exports = function(context) {
+  const isExperimentOn = 'CallExpression[callee.name=isExperimentOn]';
+  const message = 'isExperimentOn must be passed an explicit string';
+
+  return {
+    [isExperimentOn](node) {
+      const arg = node.arguments[1];
+      if (!arg) {
+        context.report({node, message});
+        return;
+      }
+
+      const comments = context.getCommentsBefore(arg);
+      const ok = comments.some(comment => comment.value === 'OK');
+      if (ok) {
+        return;
+      }
+
+      if (arg.type === 'Literal' && typeof arg.value === 'string') {
+        return;
+      }
+
+      context.report({node, message});
+    },
+  };
+};
diff --git a/extensions/amp-access/0.1/amp-access-server-jwt.js b/extensions/amp-access/0.1/amp-access-server-jwt.js
@@ -109,7 +109,7 @@ export class AccessServerJwtAdapter {
     this.serverState_ = stateElement ?
       stateElement.getAttribute('content') : null;
 
-    const isInExperiment = isExperimentOn(ampdoc.win, TAG);
+    const isInExperiment = isExperimentOn(ampdoc.win, 'amp-access-server-jwt');
 
     /** @private @const {boolean} */
     this.isProxyOrigin_ = isProxyOrigin(ampdoc.win.location) || isInExperiment;

diff --git a/extensions/amp-access/0.1/amp-access-server.js b/extensions/amp-access/0.1/amp-access-server.js
@@ -92,7 +92,7 @@ export class AccessServerAdapter {
     this.serverState_ = stateElement ?
       stateElement.getAttribute('content') : null;
 
-    const isInExperiment = isExperimentOn(ampdoc.win, TAG);
+    const isInExperiment = isExperimentOn(ampdoc.win, 'amp-access-server');
 
     /** @private @const {boolean} */
     this.isProxyOrigin_ = isProxyOrigin(ampdoc.win.location) || isInExperiment;

diff --git a/extensions/amp-access/0.1/signin.js b/extensions/amp-access/0.1/signin.js
@@ -75,7 +75,7 @@ export class SignInProtocol {
 
     /** @private @const {boolean} */
     this.isEnabled_ =
-        isExperimentOn(ampdoc.win, TAG) &&
+        isExperimentOn(ampdoc.win, 'amp-access-signin') &&
         this.viewer_.isEmbedded() &&
         this.viewer_.getParam('signin') == '1';
 

diff --git a/extensions/amp-analytics/0.1/test/test-requests.js b/extensions/amp-analytics/0.1/test/test-requests.js
@@ -16,7 +16,6 @@
 
 import * as lolex from 'lolex';
 import {ExpansionOptions, installVariableService} from '../variables';
-import {REPLACEMENT_EXP_NAME} from '../../../../src/service/url-replacements-impl';
 import {RequestHandler, expandConfigRequest} from '../requests';
 import {dict} from '../../../../src/utils/object';
 import {macroTask} from '../../../../testing/yield';
@@ -481,7 +480,7 @@ describes.realWin('Requests', {amp: 1}, env => {
 
 
   it('should replace bindings with v2 flag', function* () {
-    toggleExperiment(env.win, REPLACEMENT_EXP_NAME, true);
+    toggleExperiment(env.win, 'url-replacement-v2', true);
     const spy = sandbox.spy();
     const r = {
       'baseUrl': 'r1&${extraUrlParams}&BASE_VALUE&foo=${foo}',
@@ -512,6 +511,6 @@ describes.realWin('Requests', {amp: 1}, env => {
     expect(spy).to.be.calledOnce;
     expect(spy.args[0][0]).to.equal(
         'r1&key1=val1&key2=val2&key3=val3&val_base&foo=ZM9V');
-    toggleExperiment(env.win, REPLACEMENT_EXP_NAME);
+    toggleExperiment(env.win, 'url-replacement-v2');
   });
 });
diff --git a/extensions/amp-analytics/0.1/test/test-variables.js b/extensions/amp-analytics/0.1/test/test-variables.js
@@ -21,7 +21,6 @@ import {
   installVariableService,
   variableServiceFor,
 } from '../variables';
-import {REPLACEMENT_EXP_NAME} from '../../../../src/service/url-replacements-impl';
 import {Services} from '../../../../src/services';
 import {adopt} from '../../../../src/runtime';
 import {toggleExperiment} from '../../../../src/experiments';
@@ -143,14 +142,14 @@ describe('amp-analytics.VariableService', function() {
     beforeEach(() => {
       ampdoc = env.ampdoc;
       win = env.win;
-      toggleExperiment(env.win, REPLACEMENT_EXP_NAME, true);
+      toggleExperiment(env.win, 'url-replacement-v2', true);
       installVariableService(win);
       variables = variableServiceFor(win);
       urlReplacementService = Services.urlReplacementsForDoc(ampdoc);
     });
 
     afterEach(() => {
-      toggleExperiment(env.win, REPLACEMENT_EXP_NAME);
+      toggleExperiment(env.win, 'url-replacement-v2');
     });
 
     function check(input, output) {

diff --git a/extensions/amp-analytics/0.1/variables.js b/extensions/amp-analytics/0.1/variables.js
@@ -14,7 +14,6 @@
  * limitations under the License.
  */
 
-import {REPLACEMENT_EXP_NAME} from '../../../src/service/url-replacements-impl';
 import {Services} from '../../../src/services';
 import {dev, user} from '../../../src/log';
 import {getService, registerServiceBuilder} from '../../../src/service';
@@ -151,7 +150,7 @@ export class VariableService {
    */
   getMacros() {
     const isV2ExpansionOn = this.win_ && isExperimentOn(this.win_,
-        REPLACEMENT_EXP_NAME);
+        'url-replacement-v2');
     return isV2ExpansionOn ? this.macros_ : {};
   }
 

diff --git a/extensions/amp-auto-ads/0.1/amp-auto-ads.js b/extensions/amp-auto-ads/0.1/amp-auto-ads.js
@@ -39,7 +39,8 @@ export class AmpAutoAds extends AMP.BaseElement {
 
   /** @override */
   buildCallback() {
-    user().assert(isExperimentOn(this.win, TAG), 'Experiment is off');
+    user().assert(isExperimentOn(this.win, 'amp-auto-ads'),
+        'Experiment is off');
 
     const type = this.element.getAttribute('type');
     user().assert(type, 'Missing type attribute');

diff --git a/extensions/amp-consent/0.1/amp-consent.js b/extensions/amp-consent/0.1/amp-consent.js
@@ -17,10 +17,7 @@
 import {CONSENT_ITEM_STATE, ConsentStateManager} from './consent-state-manager';
 import {CONSENT_POLICY_STATE} from '../../../src/consent-state';
 import {CSS} from '../../../build/amp-consent-0.1.css';
-import {
-  ConsentPolicyManager,
-  MULTI_CONSENT_EXPERIMENT,
-} from './consent-policy-manager';
+import {ConsentPolicyManager} from './consent-policy-manager';
 import {Deferred} from '../../../src/utils/promise';
 import {
   NOTIFICATION_UI_MANAGER,
@@ -38,7 +35,6 @@ import {dict, map} from '../../../src/utils/object';
 import {getData} from '../../../src/event-helper';
 import {getServicePromiseForDoc} from '../../../src/service';
 import {isEnumValue} from '../../../src/types';
-import {isExperimentOn} from '../../../src/experiments';
 import {parseJson} from '../../../src/json';
 import {setImportantStyles, toggle} from '../../../src/style';
 
@@ -128,7 +124,7 @@ export class AmpConsent extends AMP.BaseElement {
 
   /** @override */
   buildCallback() {
-    this.isMultiSupported_ = isExperimentOn(this.win, MULTI_CONSENT_EXPERIMENT);
+    this.isMultiSupported_ = ConsentPolicyManager.isMultiSupported(this.win);
 
     user().assert(this.element.getAttribute('id'),
         'amp-consent should have an id');

diff --git a/extensions/amp-consent/0.1/consent-policy-manager.js b/extensions/amp-consent/0.1/consent-policy-manager.js
@@ -24,7 +24,6 @@ import {isExperimentOn} from '../../../src/experiments';
 import {isFiniteNumber} from '../../../src/types';
 import {isObject} from '../../../src/types';
 
-export const MULTI_CONSENT_EXPERIMENT = 'multi-consent';
 const CONSENT_STATE_MANAGER = 'consentStateManager';
 const TAG = 'consent-policy-manager';
 
@@ -68,6 +67,16 @@ export class ConsentPolicyManager {
 
   }
 
+  /**
+   * Is Multi-consent experiment enabled?
+   *
+   * @param {!Window} win
+   * @return {boolean}
+   */
+  static isMultiSupported(win) {
+    return isExperimentOn(win, 'multi-consent');
+  }
+
   /**
    * Register the policy instance
    * Example policy config format:
@@ -150,7 +159,7 @@ export class ConsentPolicyManager {
    * @return {!Promise<CONSENT_POLICY_STATE>}
    */
   whenPolicyResolved(policyId) {
-    if (!isExperimentOn(this.ampdoc_.win, MULTI_CONSENT_EXPERIMENT)) {
+    if (!ConsentPolicyManager.isMultiSupported(this.ampdoc_.win)) {
       // If customized policy is not supported
       if (!WHITELIST_POLICY[policyId]) {
         user().error(TAG, `can not find defined policy ${policyId}, ` +
@@ -171,7 +180,7 @@ export class ConsentPolicyManager {
    * @return {!Promise<boolean>}
    */
   whenPolicyUnblock(policyId) {
-    if (!isExperimentOn(this.ampdoc_.win, MULTI_CONSENT_EXPERIMENT)) {
+    if (!ConsentPolicyManager.isMultiSupported(this.ampdoc_.win)) {
       // If customized policy is not supported
       if (!WHITELIST_POLICY[policyId]) {
         user().error(TAG, `can not find defined policy ${policyId}, ` +

diff --git a/extensions/amp-google-document-embed/0.1/amp-google-document-embed.js b/extensions/amp-google-document-embed/0.1/amp-google-document-embed.js
@@ -75,8 +75,8 @@ export class AmpDriveViewer extends AMP.BaseElement {
 
   /** @override */
   buildCallback() {
-    user().assert(
-        isExperimentOn(this.win, TAG), `Experiment ${TAG} is disabled`);
+    user().assert(isExperimentOn(this.win, 'amp-google-document-embed'),
+        'Experiment amp-google-document-embed is disabled');
     user().assert(
         this.element.getAttribute('src'),
         'The src attribute is required for <amp-google-document-embed> %s',

diff --git a/extensions/amp-google-vrview-image/0.1/amp-google-vrview-image.js b/extensions/amp-google-vrview-image/0.1/amp-google-vrview-image.js
@@ -15,9 +15,9 @@
  */
 
 import {addParamToUrl, assertHttpsUrl} from '../../../src/url';
-import {dev} from '../../../src/log';
 import {isExperimentOn} from '../../../src/experiments';
 import {isLayoutSizeDefined} from '../../../src/layout';
+import {user} from '../../../src/log';
 
 /** @const */
 const TAG = 'amp-google-vrview-image';
@@ -29,9 +29,6 @@ class AmpGoogleVrviewImage extends AMP.BaseElement {
   constructor(element) {
     super(element);
 
-    /** @private {boolean} */
-    this.isExperimentOn_ = false;
-
     /** @private {string} */
     this.imageSrc_ = '';
 
@@ -47,11 +44,8 @@ class AmpGoogleVrviewImage extends AMP.BaseElement {
 
   /** @override */
   buildCallback() {
-    this.isExperimentOn_ = isExperimentOn(this.win, TAG);
-    if (!this.isExperimentOn_) {
-      dev().warn(TAG, `TAG ${TAG} disabled`);
-      return;
-    }
+    user().assert(isExperimentOn(this.win, 'amp-google-vrview-image'),
+        'TAG amp-google-vrview-image disabled');
 
     this.imageSrc_ = assertHttpsUrl(this.element.getAttribute('src'),
         this.element);
@@ -90,12 +84,6 @@ class AmpGoogleVrviewImage extends AMP.BaseElement {
 
   /** @override */
   layoutCallback() {
-    this.isExperimentOn_ = isExperimentOn(this.win, TAG);
-    if (!this.isExperimentOn_) {
-      dev().warn(TAG, `TAG ${TAG} disabled`);
-      return Promise.resolve();
-    }
-
     const iframe = this.element.ownerDocument.createElement('iframe');
     iframe.onload = () => {
       // Chrome does not reflect the iframe readystate.

diff --git a/extensions/amp-next-page/0.1/amp-next-page.js b/extensions/amp-next-page/0.1/amp-next-page.js
@@ -50,7 +50,8 @@ export class AmpNextPage extends AMP.BaseElement {
 
   /** @override */
   buildCallback() {
-    user().assert(isExperimentOn(this.win, TAG), `Experiment ${TAG} disabled`);
+    user().assert(isExperimentOn(this.win, 'amp-next-page'),
+        'Experiment amp-next-page disabled');
 
     if (this.service_.isActive()) {
       return;

diff --git a/extensions/amp-playbuzz/0.1/amp-playbuzz.js b/extensions/amp-playbuzz/0.1/amp-playbuzz.js
@@ -53,8 +53,6 @@ import {isExperimentOn} from '../../../src/experiments';
 import {logo, showMoreArrow} from './images';
 import {removeElement} from '../../../src/dom';
 import {user} from '../../../src/log';
-/** @const */
-const EXPERIMENT = 'amp-playbuzz';
 
 class AmpPlaybuzz extends AMP.BaseElement {
 
@@ -105,8 +103,8 @@ class AmpPlaybuzz extends AMP.BaseElement {
   buildCallback() {
     // EXPERIMENT
     // AMP.toggleExperiment(EXPERIMENT, true); //for dev
-    user().assert(isExperimentOn(this.win, EXPERIMENT),
-        `Enable ${EXPERIMENT} experiment`);
+    user().assert(isExperimentOn(this.win, 'amp-playbuzz'),
+        'Enable amp-playbuzz experiment');
 
     const e = this.element;
     const src = e.getAttribute('src');

diff --git a/extensions/amp-share-tracking/0.1/amp-share-tracking.js b/extensions/amp-share-tracking/0.1/amp-share-tracking.js
@@ -51,7 +51,7 @@ export class AmpShareTracking extends AMP.BaseElement {
     * @private
     */
   isExperimentOn_() {
-    return isExperimentOn(this.win, TAG);
+    return isExperimentOn(this.win, 'amp-share-tracking');
   }
 
   /** @override */

diff --git a/extensions/amp-story/0.1/amp-story.js b/extensions/amp-story/0.1/amp-story.js
@@ -678,7 +678,7 @@ export class AmpStory extends AMP.BaseElement {
 
   /** @private */
   isAmpStoryEnabled_() {
-    if (isExperimentOn(this.win, TAG) || getMode().test ||
+    if (isExperimentOn(this.win, 'amp-story') || getMode().test ||
         this.win.location.protocol === 'file:') {
       return true;
     }

diff --git a/extensions/amp-viz-vega/0.1/amp-viz-vega.js b/extensions/amp-viz-vega/0.1/amp-viz-vega.js
@@ -24,9 +24,6 @@ import {isFiniteNumber, isObject} from '../../../src/types';
 import {isLayoutSizeDefined} from '../../../src/layout';
 import {tryParseJson} from '../../../src/json';
 
-/** @const */
-const EXPERIMENT = 'amp-viz-vega';
-
 export class AmpVizVega extends AMP.BaseElement {
 
 /** @param {!AmpElement} element */
@@ -74,8 +71,8 @@ export class AmpVizVega extends AMP.BaseElement {
 
   /** @override */
   buildCallback() {
-    user().assert(isExperimentOn(this.win, EXPERIMENT),
-        `Experiment ${EXPERIMENT} disabled`);
+    user().assert(isExperimentOn(this.win, 'amp-viz-vega'),
+        'Experiment amp-viz-vega disabled');
 
     /**
      * Global vg (and implicitly d3) are required and they are created by

diff --git a/src/experiments.js b/src/experiments.js
@@ -88,7 +88,7 @@ export function isExperimentOn(win, experimentId) {
  */
 export function toggleExperiment(win, experimentId, opt_on,
   opt_transientExperiment) {
-  const currentlyOn = isExperimentOn(win, experimentId);
+  const currentlyOn = isExperimentOn(win, /*OK*/experimentId);
   const on = !!(opt_on !== undefined ? opt_on : !currentlyOn);
   if (on != currentlyOn) {
     const toggles = experimentToggles(win);
@@ -316,7 +316,7 @@ export function randomlySelectUnsetExperiments(win, experiments) {
     // experiment branch (e.g., via a test setup), then randomize the branch
     // choice.
     if (!win.experimentBranches[experimentName] &&
-        isExperimentOn(win, experimentName)) {
+      isExperimentOn(win, /*OK*/experimentName)) {
       const {branches} = experiments[experimentName];
       win.experimentBranches[experimentName] = selectRandomItem(branches);
       selectedExperiments[experimentName] =