3p-frame: Prefer #core/mode to getMode (#35669)

* Prefer #core/mode to getMode * Set isProd explicitly in `mockMode` * Only need mode.isLocalDev() * Stub `isTest` anyway * Copy `mockMode` to amp-ad-3p-impl.js * Rely on `!isProd()` over `isLocalDev()`
ampproject · Aug 13, 2021 · 07771ad · 07771ad
1 parent 0d3a7cb
commit 07771ad
Show file tree

Hide file tree

Showing 3 changed files with 19 additions and 21 deletions.
diff --git a/extensions/amp-ad/0.1/test/test-amp-ad-3p-impl.js b/extensions/amp-ad/0.1/test/test-amp-ad-3p-impl.js
@@ -18,6 +18,7 @@ import '../../../amp-sticky-ad/1.0/amp-sticky-ad';
 import '../amp-ad';
 import * as adCid from '../../../../src/ad-cid';
 import * as consent from '../../../../src/consent';
+import * as mode from '#core/mode';
 import * as fakeTimers from '@sinonjs/fake-timers';
 import {AmpAd3PImpl} from '../amp-ad-3p-impl';
 import {AmpAdUIHandler} from '../amp-ad-ui';
@@ -69,10 +70,6 @@ describes.realWin(
     let registryBackup;
     const whenFirstVisible = Promise.resolve();
 
-    function mockMode(mode) {
-      env.sandbox.stub(win.parent, '__AMP_MODE').value(mode);
-    }
-
     beforeEach(() => {
       registryBackup = Object.create(null);
       Object.keys(adConfig).forEach((k) => {
@@ -331,7 +328,7 @@ describes.realWin(
 
     describe('preconnectCallback', () => {
       it('should add preconnect and prefetch to DOM header', () => {
-        mockMode({});
+        env.sandbox.stub(mode, 'isProd').returns(true);
         ad3p.buildCallback();
         ad3p.preconnectCallback();
         return whenFirstVisible.then(() => {

diff --git a/src/3p-frame.js b/src/3p-frame.js
@@ -23,7 +23,6 @@ import {tryParseJson} from '#core/types/object/json';
 import {urls} from './config';
 import {getContextMetadata} from './iframe-attributes';
 import {dev, devAssert, user, userAssert} from './log';
-import {getMode} from './mode';
 import {assertHttpsUrl, parseUrlDeprecated} from './url';
 
 /** @type {!Object<string,number>} Number of 3p frames on the for that type. */
@@ -203,13 +202,13 @@ export function addDataAndJsonAttributes_(element, attributes) {
  */
 export function getBootstrapUrl(type) {
   const extension = IS_ESM ? '.mjs' : '.js';
-  if (getMode().localDev || getMode().test) {
-    const filename = mode.isMinified()
-      ? `./vendor/${type}`
-      : `./vendor/${type}.max`;
-    return filename + extension;
+  if (mode.isProd()) {
+    return `${urls.thirdParty}/${mode.version()}/vendor/${type}${extension}`;
   }
-  return `${urls.thirdParty}/${mode.version()}/vendor/${type}${extension}`;
+  const filename = mode.isMinified()
+    ? `./vendor/${type}`
+    : `./vendor/${type}.max`;
+  return filename + extension;
 }
 
 /**
@@ -269,7 +268,7 @@ export function resetBootstrapBaseUrlForTesting(win) {
  */
 export function getDefaultBootstrapBaseUrl(parentWindow, opt_srcFileBasename) {
   const srcFileBasename = opt_srcFileBasename || 'frame';
-  if (getMode().localDev || getMode().test) {
+  if (!mode.isProd()) {
     return getDevelopmentBootstrapBaseUrl(parentWindow, srcFileBasename);
   }
   // Ensure same sub-domain is used despite potentially different file.

diff --git a/test/unit/3p/test-3p-frame.js b/test/unit/3p/test-3p-frame.js
@@ -16,6 +16,7 @@
 
 import {deserializeMessage, serializeMessage} from '#core/3p-frame-messaging';
 import {DomFingerprint} from '#core/dom/fingerprint';
+import * as mode from '#core/mode';
 import {WindowInterface} from '#core/window/interface';
 
 import {toggleExperiment} from '#experiments';
@@ -42,8 +43,9 @@ describes.realWin('3p-frame', {amp: true}, (env) => {
     document = window.document;
   });
 
-  function mockMode(mode) {
-    env.sandbox.stub(window.parent, '__AMP_MODE').value(mode);
+  function mockMode(options) {
+    env.sandbox.stub(window.parent, '__AMP_MODE').value(options);
+    env.sandbox.stub(mode, 'isProd').returns(!!options.isProd);
   }
 
   describe
@@ -336,23 +338,23 @@ describes.realWin('3p-frame', {amp: true}, (env) => {
       });
 
       it('should pick the right bootstrap unique url (prod)', () => {
-        mockMode({});
+        mockMode({isProd: true});
         const ampdoc = Services.ampdoc(window.document);
         expect(getBootstrapBaseUrl(window, ampdoc)).to.match(
           /^https:\/\/d-\d+\.ampproject\.net\/\$\internal\w+\$\/frame\.html$/
         );
       });
 
       it('should return a stable URL in getBootstrapBaseUrl', () => {
-        mockMode({});
+        mockMode({isProd: true});
         const ampdoc = Services.ampdoc(window.document);
         expect(getBootstrapBaseUrl(window, ampdoc)).to.equal(
           getBootstrapBaseUrl(window, ampdoc)
         );
       });
 
       it('should return a stable URL in getDefaultBootstrapBaseUrl', () => {
-        mockMode({});
+        mockMode({isProd: true});
         expect(getDefaultBootstrapBaseUrl(window)).to.equal(
           getDefaultBootstrapBaseUrl(window)
         );
@@ -367,7 +369,7 @@ describes.realWin('3p-frame', {amp: true}, (env) => {
       });
 
       it('should return different values for different file names', () => {
-        mockMode({});
+        mockMode({isProd: true});
         let match =
           /^https:\/\/(d-\d+\.ampproject\.net)\/\$\internal\w+\$\/frame\.html$/.exec(
             getDefaultBootstrapBaseUrl(window)
@@ -414,7 +416,7 @@ describes.realWin('3p-frame', {amp: true}, (env) => {
       });
 
       it('should prefetch bootstrap frame and JS', () => {
-        mockMode({});
+        mockMode({isProd: true});
         const ampdoc = Services.ampdoc(window.document);
         preloadBootstrap(window, 'avendor', ampdoc, preconnect);
         // Wait for visible promise.
@@ -475,7 +477,7 @@ describes.realWin('3p-frame', {amp: true}, (env) => {
           .returns('http://acme.org/')
           .twice();
 
-        mockMode({});
+        mockMode({isProd: true});
         const link = document.createElement('link');
         link.setAttribute('rel', 'canonical');
         link.setAttribute('href', 'https://foo.bar/baz');