♻️ Define helper for hash params and drop barely-used getMode().log (ampproject#35628)

rcebulko · web-flow · commit b9e41a2c3c2d · 2021-08-12T20:47:01.000Z
* Add getHashParams helper

* Add win param

* Update mode.js to use helper

* Add tests

* Update callsites

* Remove getMode().log uses

* Remove log from ModeDef type

* Lint fixes

* Fix tests

* Split up isModeDevelopment
diff --git a/extensions/amp-a4a/0.1/amp-a4a.js b/extensions/amp-a4a/0.1/amp-a4a.js
@@ -42,7 +42,7 @@ import {
   createSecureFrame,
   isAttributionReportingSupported,
 } from './secure-frame';
-import {dev, devAssert, user, userAssert} from '../../../src/log';
+import {dev, devAssert, logHashParam, user, userAssert} from '../../../src/log';
 import {dict} from '#core/types/object';
 import {duplicateErrorIfNecessary} from '#core/error';
 import {
@@ -1323,7 +1323,7 @@ export class AmpA4A extends AMP.BaseElement {
     // Additional arguments.
     assignAdUrlToError(/** @type {!Error} */ (error), this.adUrl_);
 
-    if (getMode().development || getMode().localDev || getMode().log) {
+    if (getMode().development || getMode().localDev || logHashParam()) {
       user().error(TAG, error);
     } else {
       user().warn(TAG, error);
diff --git a/src/core/types/string/url.js b/src/core/types/string/url.js
@@ -57,3 +57,15 @@ export function parseQueryString(queryString) {
   }
   return params;
 }
+
+/**
+ * Parses the query # params.
+ * @param {!Window=} opt_win
+ * @return {!JsonObject}
+ */
+export function getHashParams(opt_win) {
+  const {location} = opt_win || self;
+  // location.originalHash is set by the viewer when it removes the fragment
+  // from the URL.
+  return parseQueryString(location['originalHash'] || location.hash);
+}
diff --git a/src/log.js b/src/log.js
@@ -30,6 +30,7 @@ import {
 import * as mode from '#core/mode';
 import {isArray, isString} from '#core/types';
 import {once} from '#core/types/function';
+import {getHashParams} from '#core/types/string/url';
 
 import {urls} from './config';
 import {getMode} from './mode';
@@ -108,6 +109,13 @@ const externalMessagesSimpleTableUrl = () =>
 const messageArgToEncodedComponent = (arg) =>
   encodeURIComponent(String(elementStringOrPassThru(arg)));
 
+/**
+ * @param {!Window=} opt_win
+ * @return {number}
+ */
+export const logHashParam = (opt_win) =>
+  parseInt(getHashParams(opt_win)['log'], 10);
+
 /**
  * Logging class. Use of sentinel string instead of a boolean to check user/dev
  * errors because errors could be rethrown by some native code as a new error,
@@ -176,17 +184,18 @@ export class Log {
    * @private
    */
   defaultLevel_() {
+    const {win} = this;
     // No console - can't enable logging.
     if (
-      !this.win.console?.log ||
+      !win.console?.log ||
       // Logging has been explicitly disabled.
-      getMode().log == 0
+      logHashParam(win) == 0
     ) {
       return LogLevel.OFF;
     }
 
     // Logging is enabled for tests directly.
-    if (getMode().test && this.win.ENABLE_LOG) {
+    if (getMode().test && win.ENABLE_LOG) {
       return LogLevel.FINE;
     }
 
@@ -199,12 +208,13 @@ export class Log {
   }
 
   /**
+   * @param {!Window=} opt_win provided for testing
    * @return {!LogLevel}
    * @private
    */
-  defaultLevelWithFunc_() {
+  defaultLevelWithFunc_(opt_win) {
     // Delegate to the specific resolver.
-    return this.levelFunc_(getMode().log, getMode().development);
+    return this.levelFunc_(logHashParam(opt_win), getMode().development);
   }
 
   /**
diff --git a/src/mode-object.js b/src/mode-object.js
@@ -28,7 +28,6 @@ export function getModeObject(opt_win) {
     development: getMode(opt_win).development,
     esm: IS_ESM,
     test: getMode(opt_win).test,
-    log: getMode(opt_win).log,
     rtvVersion: getMode(opt_win).rtvVersion,
   };
 }
diff --git a/src/mode.js b/src/mode.js
@@ -15,14 +15,13 @@
  */
 
 import * as coreMode from '#core/mode';
-import {parseQueryString} from '#core/types/string/url';
+import {getHashParams} from '#core/types/string/url';
 
 /**
  * @typedef {{
  *   localDev: boolean,
  *   development: boolean,
  *   test: boolean,
- *   log: (string|undefined),
  *   rtvVersion: string,
  *   runtime: (null|string|undefined),
  *   a4aId: (null|string|undefined),
@@ -57,24 +56,19 @@ export function getMode(opt_win) {
  * @return {!ModeDef}
  */
 function getMode_(win) {
-  const hashQuery = parseQueryString(
-    // location.originalHash is set by the viewer when it removes the fragment
-    // from the URL.
-    win.location['originalHash'] || win.location.hash
-  );
+  const hashParams = getHashParams(win);
 
   // The `minified`, `test` and `localDev` properties are replaced
   // as boolean literals when we run `amp dist` without the `--fortesting`
   // flags. This improved DCE on the production file we deploy as the code
   // paths for localhost/testing/development are eliminated.
   return {
     localDev: coreMode.isLocalDev(win),
-    development: isModeDevelopment(win),
+    development: isModeDevelopment(win, hashParams),
     esm: IS_ESM,
     // amp-geo override
-    geoOverride: hashQuery['amp-geo'],
+    geoOverride: hashParams['amp-geo'],
     test: coreMode.isTest(win),
-    log: parseInt(hashQuery['log'], 10),
     rtvVersion: getRtvVersion(win),
   };
 }
@@ -104,17 +98,13 @@ function getRtvVersion(win) {
  * bypassed via #validate=0.
  * Note that AMP_DEV_MODE flag is used for testing purposes.
  * @param {!Window} win
+ * @param {!JsonObject=} opt_hashParams
  * @return {boolean}
  */
-export function isModeDevelopment(win) {
-  const hashQuery = parseQueryString(
-    win.location['originalHash'] || win.location.hash
-  );
-  return !!(
-    ['1', 'actions', 'amp', 'amp4ads', 'amp4email'].includes(
-      hashQuery['development']
-    ) || win.AMP_DEV_MODE
-  );
+export function isModeDevelopment(win, opt_hashParams) {
+  const devModes = ['1', 'actions', 'amp', 'amp4ads', 'amp4email'];
+  const devParam = opt_hashParams || getHashParams(win);
+  return devModes.includes(devParam['development']) || !!win.AMP_DEV_MODE;
 }
 
 /**
diff --git a/src/service/url-replacements-impl.js b/src/service/url-replacements-impl.js
@@ -16,7 +16,7 @@
 
 import * as mode from '#core/mode';
 import {hasOwn} from '#core/types/object';
-import {parseQueryString} from '#core/types/string/url';
+import {getHashParams, parseQueryString} from '#core/types/string/url';
 import {WindowInterface} from '#core/window/interface';
 
 import {Services} from '#service';
@@ -751,8 +751,7 @@ export class GlobalVariableSource extends VariableSource {
         'param is required'
     );
     userAssert(typeof param == 'string', 'param should be a string');
-    const hash = this.ampdoc.win.location['originalHash'];
-    const params = parseQueryString(hash);
+    const params = getHashParams(this.ampdoc.win);
     return params[param] === undefined ? defaultValue : params[param];
   }
 
diff --git a/src/validator-integration.js b/src/validator-integration.js
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import {parseQueryString} from '#core/types/string/url';
+import {getHashParams} from '#core/types/string/url';
 
 import {urls} from './config';
 import {loadPromise} from './event-helper';
@@ -34,11 +34,9 @@ export function maybeValidate(win) {
     return;
   }
   let validator = false;
-  if (isModeDevelopment(win)) {
-    const hash = parseQueryString(
-      win.location['originalHash'] || win.location.hash
-    );
-    validator = hash['validate'] !== '0';
+  const params = getHashParams(win);
+  if (isModeDevelopment(win, params)) {
+    validator = params['validate'] !== '0';
   }
 
   if (validator) {
diff --git a/test/unit/core/types/string/test-url.js b/test/unit/core/types/string/test-url.js
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import {parseQueryString} from '#core/types/string/url';
+import {getHashParams, parseQueryString} from '#core/types/string/url';
 
 describes.sandboxed('type helpers - strings - urls', {}, () => {
   describe('parseQueryString', () => {
@@ -68,4 +68,31 @@ describes.sandboxed('type helpers - strings - urls', {}, () => {
       });
     });
   });
+
+  describe('getHashParams', () => {
+    it('parses the `originalHash`', () => {
+      const params = getHashParams({
+        location: {
+          originalHash: '#development=1&original',
+          hash: '#development=1&missingOriginal',
+        },
+      });
+
+      expect(params).to.deep.equal({
+        'development': '1',
+        'original': '',
+      });
+    });
+
+    it('parses the hash if `originalHash` is unset', () => {
+      const params = getHashParams({
+        location: {hash: '#development=1&missingOriginal'},
+      });
+
+      expect(params).to.deep.equal({
+        'development': '1',
+        'missingOriginal': '',
+      });
+    });
+  });
 });
diff --git a/test/unit/test-log.js b/test/unit/test-log.js
@@ -83,7 +83,7 @@ describes.sandboxed('Logging', {}, (env) => {
     });
 
     it('should be disabled with hash param log=0', () => {
-      mode.log = '0';
+      win.location.hash = '#log=0';
       expect(new Log(win, RETURNS_FINE).level_).to.equal(LogLevel.OFF);
     });
 
@@ -235,19 +235,19 @@ describes.sandboxed('Logging', {}, (env) => {
     });
 
     it('should be enabled with log=1', () => {
-      mode.log = '1';
-      expect(user().defaultLevelWithFunc_()).to.equal(LogLevel.FINE);
+      win.location.hash = '#log=1';
+      expect(user().defaultLevelWithFunc_(win)).to.equal(LogLevel.FINE);
     });
 
     it('should be enabled with log>1', () => {
-      mode.log = '2';
-      expect(user().defaultLevelWithFunc_()).to.equal(LogLevel.FINE);
+      win.location.hash = '#log=2';
+      expect(user().defaultLevelWithFunc_(win)).to.equal(LogLevel.FINE);
 
-      mode.log = '3';
-      expect(user().defaultLevelWithFunc_()).to.equal(LogLevel.FINE);
+      win.location.hash = '#log=3';
+      expect(user().defaultLevelWithFunc_(win)).to.equal(LogLevel.FINE);
 
-      mode.log = '4';
-      expect(user().defaultLevelWithFunc_()).to.equal(LogLevel.FINE);
+      win.location.hash = '#log=4';
+      expect(user().defaultLevelWithFunc_(win)).to.equal(LogLevel.FINE);
     });
 
     it('should be configured with USER suffix', () => {
@@ -266,18 +266,18 @@ describes.sandboxed('Logging', {}, (env) => {
     });
 
     it('should NOT be enabled with log=1', () => {
-      mode.log = 1;
-      expect(dev().defaultLevelWithFunc_()).to.equal(LogLevel.OFF);
+      win.location.hash = '#log=1';
+      expect(dev().defaultLevelWithFunc_(win)).to.equal(LogLevel.OFF);
     });
 
     it('should be enabled as INFO with log=2', () => {
-      mode.log = 2;
-      expect(dev().defaultLevelWithFunc_()).to.equal(LogLevel.INFO);
+      win.location.hash = '#log=2';
+      expect(dev().defaultLevelWithFunc_(win)).to.equal(LogLevel.INFO);
     });
 
     it('should be enabled as FINE with log=3', () => {
-      mode.log = 3;
-      expect(dev().defaultLevelWithFunc_()).to.equal(LogLevel.FINE);
+      win.location.hash = '#log=3';
+      expect(dev().defaultLevelWithFunc_(win)).to.equal(LogLevel.FINE);
     });
 
     it('should be configured with no suffix', () => {

Original file line number	Diff line number	Diff line change
`@@ -28,7 +28,6 @@ export function getModeObject(opt_win) {`
`28`	`28`	`development: getMode(opt_win).development,`
`29`	`29`	`esm: IS_ESM,`
`30`	`30`	`test: getMode(opt_win).test,`
`31`		`- log: getMode(opt_win).log,`
`32`	`31`	`rtvVersion: getMode(opt_win).rtvVersion,`
`33`	`32`	`};`
`34`	`33`	`}`