Redefine getMode().test to only be true in local development (#28072)

* Redefine getMode().test to only be true in local development Right now in production, we still include all of the code guarded by `getMode().test`. That's because it's a configuration value and not a compile time constant. Now, test will be stripped out when running `gulp dist` if you do not include `--fortesting`. * Cleanup getMode usage * Fixes * Quick and dirty test fix
ampproject · Apr 29, 2020 · d7480aa · d7480aa
1 parent 67815dc
commit d7480aa
Show file tree

Hide file tree

Showing 10 changed files with 89 additions and 49 deletions.
diff --git a/.eslintrc b/.eslintrc
@@ -89,6 +89,7 @@
     "local/await-expect": 2,
     "local/closure-type-primitives": 2,
     "local/dict-string-keys": 2,
+    "local/get-mode-usage": 2,
     "local/html-template": 2,
     "local/is-experiment-on": 2,
     "local/json-configuration": 2,

diff --git a/build-system/babel-config/pre-closure-config.js b/build-system/babel-config/pre-closure-config.js
@@ -83,7 +83,7 @@ function getPreClosureConfig() {
     !isCheckTypes
       ? './build-system/babel-plugins/babel-plugin-transform-json-configuration'
       : null,
-    argv.esm
+    !(argv.fortesting || isCheckTypes)
       ? [
           './build-system/babel-plugins/babel-plugin-amp-mode-transformer',
           {isEsmBuild: !!argv.esm},

diff --git a/build-system/eslint-rules/get-mode-usage.js b/build-system/eslint-rules/get-mode-usage.js
@@ -0,0 +1,47 @@
+/**
+ * Copyright 2020 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';
+
+// This rule asserts that we only grab properties from getMode(), and never
+// store a reference to the return value.
+//
+// Good:
+// getMode().foo;
+// const x = getMode().test;
+//
+// Bad:
+// const mode = getMode();
+// isTest(getMode());
+// obj[getMode()];
+module.exports = function (context) {
+  return {
+    'CallExpression[callee.name=getMode]': function (node) {
+      if (
+        node.parent.type === 'MemberExpression' &&
+        node.parent.object === node
+      ) {
+        return;
+      }
+
+      context.report({
+        node,
+        message:
+          'Do not re-alias getMode or its return value so it can be ' +
+          "DCE'd. Use explicitly like `getMode().localDev` instead.",
+      });
+    },
+  };
+};
diff --git a/build-system/eslint-rules/no-import-rename.js b/build-system/eslint-rules/no-import-rename.js
@@ -47,6 +47,7 @@ const imports = {
   'src/css': ['escapeCssSelectorIdent', 'escapeCssSelectorNth'],
   'src/dom': ['scopedQuerySelector', 'scopedQuerySelectorAll'],
   'src/log': ['user', 'dev'],
+  'src/mode': ['getMode'],
 };
 
 module.exports = function (context) {

diff --git a/build-system/tasks/presubmit-checks.js b/build-system/tasks/presubmit-checks.js
@@ -144,21 +144,11 @@ const forbiddenTerms = {
     ],
     checkInTestFolder: true,
   },
-  // Match `getMode` that is not followed by a "()." and is assigned
-  // as a variable.
-  '\\bgetMode\\([^)]*\\)(?!\\.)': {
-    message: realiasGetMode,
-    whitelist: ['src/mode.js', 'dist.3p/current/integration.js'],
-  },
-  'import[^}]*\\bgetMode as': {
-    message: realiasGetMode,
-  },
   '\\bgetModeObject\\(': {
     message: realiasGetMode,
     whitelist: [
       'src/mode-object.js',
       'src/iframe-attributes.js',
-      'src/log.js',
       'dist.3p/current/integration.js',
     ],
   },

diff --git a/src/log.js b/src/log.js
@@ -15,7 +15,6 @@
  */
 
 import {getMode} from './mode';
-import {getModeObject} from './mode-object';
 import {internalRuntimeVersion} from './internal-version';
 import {isArray, isEnumValue} from './types';
 import {once} from './utils/function';
@@ -152,7 +151,7 @@ export class Log {
    * https://blog.sentry.io/2016/01/04/client-javascript-reporting-window-onerror.html
    *
    * @param {!Window} win
-   * @param {function(!./mode.ModeDef):!LogLevel} levelFunc
+   * @param {function(number, boolean):!LogLevel} levelFunc
    * @param {string=} opt_suffix
    */
   constructor(win, levelFunc, opt_suffix = '') {
@@ -163,7 +162,7 @@ export class Log {
      */
     this.win = getMode().test && win.__AMP_TEST_IFRAME ? win.parent : win;
 
-    /** @private @const {function(!./mode.ModeDef):!LogLevel} */
+    /** @private @const {function(number, boolean):!LogLevel} */
     this.levelFunc_ = levelFunc;
 
     /** @private @const {!LogLevel} */
@@ -220,8 +219,16 @@ export class Log {
       return LogLevel.INFO;
     }
 
+    return this.defaultLevelWithFunc_();
+  }
+
+  /**
+   * @return {!LogLevel}
+   * @private
+   */
+  defaultLevelWithFunc_() {
     // Delegate to the specific resolver.
-    return this.levelFunc_(getModeObject());
+    return this.levelFunc_(parseInt(getMode().log, 10), getMode().development);
   }
 
   /**
@@ -814,9 +821,8 @@ function getUserLogger(suffix) {
   }
   return new logConstructor(
     self,
-    (mode) => {
-      const logNum = parseInt(mode.log, 10);
-      if (mode.development || logNum >= 1) {
+    (logNum, development) => {
+      if (development || logNum >= 1) {
         return LogLevel.FINE;
       }
       return LogLevel.WARN;
@@ -844,8 +850,7 @@ export function dev() {
   if (!logConstructor) {
     throw new Error('failed to call initLogConstructor');
   }
-  return (logs.dev = new logConstructor(self, (mode) => {
-    const logNum = parseInt(mode.log, 10);
+  return (logs.dev = new logConstructor(self, (logNum) => {
     if (logNum >= 3) {
       return LogLevel.FINE;
     }

diff --git a/src/mode-object.js b/src/mode-object.js
@@ -27,7 +27,6 @@ export function getModeObject(opt_win) {
     localDev: getMode(opt_win).localDev,
     development: getMode(opt_win).development,
     esm: IS_ESM,
-    filter: getMode(opt_win).filter,
     minified: getMode(opt_win).minified,
     lite: getMode(opt_win).lite,
     test: getMode(opt_win).test,

diff --git a/src/mode.js b/src/mode.js
@@ -71,10 +71,9 @@ function getMode_(win) {
   const IS_DEV = true;
   const IS_MINIFIED = false;
 
-  const localDevEnabled = !!AMP_CONFIG.localDev;
   const runningTests =
-    !!AMP_CONFIG.test || (IS_DEV && !!(win.__AMP_TEST || win.__karma__));
-  const isLocalDev = IS_DEV && (localDevEnabled || runningTests);
+    IS_DEV && !!(AMP_CONFIG.test || win.__AMP_TEST || win.__karma__);
+  const isLocalDev = IS_DEV && (!!AMP_CONFIG.localDev || runningTests);
   const hashQuery = parseQueryString_(
     // location.originalHash is set by the viewer when it removes the fragment
     // from the URL.

diff --git a/test/unit/test-log.js b/test/unit/test-log.js
@@ -223,28 +223,28 @@ describe('Logging', () => {
 
   describe('UserLog', () => {
     it('should be WARN by default', () => {
-      expect(user().levelFunc_(mode)).to.equal(LogLevel.WARN);
+      expect(user().defaultLevelWithFunc_()).to.equal(LogLevel.WARN);
     });
 
     it('should be enabled in development mode', () => {
       mode.development = true;
-      expect(user().levelFunc_(mode)).to.equal(LogLevel.FINE);
+      expect(user().defaultLevelWithFunc_()).to.equal(LogLevel.FINE);
     });
 
     it('should be enabled with log=1', () => {
       mode.log = '1';
-      expect(user().levelFunc_(mode)).to.equal(LogLevel.FINE);
+      expect(user().defaultLevelWithFunc_()).to.equal(LogLevel.FINE);
     });
 
     it('should be enabled with log>1', () => {
       mode.log = '2';
-      expect(user().levelFunc_(mode)).to.equal(LogLevel.FINE);
+      expect(user().defaultLevelWithFunc_()).to.equal(LogLevel.FINE);
 
       mode.log = '3';
-      expect(user().levelFunc_(mode)).to.equal(LogLevel.FINE);
+      expect(user().defaultLevelWithFunc_()).to.equal(LogLevel.FINE);
 
       mode.log = '4';
-      expect(user().levelFunc_(mode)).to.equal(LogLevel.FINE);
+      expect(user().defaultLevelWithFunc_()).to.equal(LogLevel.FINE);
     });
 
     it('should be configured with USER suffix', () => {
@@ -254,27 +254,27 @@ describe('Logging', () => {
 
   describe('DevLog', () => {
     it('should be disabled by default', () => {
-      expect(dev().levelFunc_(mode)).to.equal(LogLevel.OFF);
+      expect(dev().defaultLevelWithFunc_()).to.equal(LogLevel.OFF);
     });
 
     it('should NOT be enabled in development mode', () => {
       mode.development = true;
-      expect(dev().levelFunc_(mode)).to.equal(LogLevel.OFF);
+      expect(dev().defaultLevelWithFunc_()).to.equal(LogLevel.OFF);
     });
 
     it('should NOT be enabled with log=1', () => {
       mode.log = '1';
-      expect(dev().levelFunc_(mode)).to.equal(LogLevel.OFF);
+      expect(dev().defaultLevelWithFunc_()).to.equal(LogLevel.OFF);
     });
 
     it('should be enabled as INFO with log=2', () => {
       mode.log = '2';
-      expect(dev().levelFunc_(mode)).to.equal(LogLevel.INFO);
+      expect(dev().defaultLevelWithFunc_()).to.equal(LogLevel.INFO);
     });
 
     it('should be enabled as FINE with log=3', () => {
       mode.log = '3';
-      expect(dev().levelFunc_(mode)).to.equal(LogLevel.FINE);
+      expect(dev().defaultLevelWithFunc_()).to.equal(LogLevel.FINE);
     });
 
     it('should be configured with no suffix', () => {

diff --git a/test/unit/test-mode.js b/test/unit/test-mode.js
@@ -32,27 +32,23 @@ describe('getMode', () => {
   it('CDN - lite mode on', () => {
     const url =
       'https://cdn.ampproject.org/v/www.example.com/amp.html?amp_js_v=5&amp_lite#origin=https://www.google.com';
-    const mode = getMode(getWin(url));
-    expect(mode.lite).to.be.true;
+    expect(getMode(getWin(url)).lite).to.be.true;
   });
 
   it('CDN - lite mode off', () => {
     const url =
       'https://cdn.ampproject.org/v/www.example.com/amp.html?amp_js_v=5#origin=https://www.google.com';
-    const mode = getMode(getWin(url));
-    expect(mode.lite).to.be.false;
+    expect(getMode(getWin(url)).lite).to.be.false;
   });
 
   it('Origin - lite mode on', () => {
     const url = 'https://www.example.com/amp.html?amp_lite';
-    const mode = getMode(getWin(url));
-    expect(mode.lite).to.be.true;
+    expect(getMode(getWin(url)).lite).to.be.true;
   });
 
   it('Origin - lite mode off', () => {
     const url = 'https://www.example.com/amp.html';
-    const mode = getMode(getWin(url));
-    expect(mode.lite).to.be.false;
+    expect(getMode(getWin(url)).lite).to.be.false;
   });
 
   it('should support different html formats for development', () => {
@@ -83,9 +79,9 @@ describe('getRtvVersion', () => {
     resetRtvVersionForTesting();
   });
 
-  function getFreshMode(win) {
+  function getFreshMode(win, prop) {
     delete win.__AMP_MODE;
-    return getMode(win);
+    return getMode(win)[prop];
   }
 
   it('should default to version', () => {
@@ -109,16 +105,18 @@ describe('getRtvVersion', () => {
       '$internalRuntimeVersion$'
     );
     expect(getRtvVersionForTesting(win, false)).to.equal('12345');
-    expect(getFreshMode(win).version).to.equal('$internalRuntimeVersion$');
+    expect(getFreshMode(win, 'version')).to.equal('$internalRuntimeVersion$');
     resetRtvVersionForTesting();
-    expect(getFreshMode(win).rtvVersion).to.equal('12345');
+    expect(getFreshMode(win, 'rtvVersion')).to.equal('12345');
 
     delete win.AMP_CONFIG;
     expect(getRtvVersionForTesting(win, false)).to.equal(
       '01$internalRuntimeVersion$'
     );
-    expect(getFreshMode(win).version).to.equal('$internalRuntimeVersion$');
+    expect(getFreshMode(win, 'version')).to.equal('$internalRuntimeVersion$');
     resetRtvVersionForTesting();
-    expect(getFreshMode(win).rtvVersion).to.equal('01$internalRuntimeVersion$');
+    expect(getFreshMode(win, 'rtvVersion')).to.equal(
+      '01$internalRuntimeVersion$'
+    );
   });
 });