Skip to content

Commit

Permalink
Revert "mode: collapse definitions of localDev and test (#35823)" (#3…
Browse files Browse the repository at this point in the history
…5918)

This reverts commit 59a07a3.
  • Loading branch information
samouri committed Sep 1, 2021
1 parent 4bdbf38 commit 1318234
Show file tree
Hide file tree
Showing 13 changed files with 38 additions and 20 deletions.
1 change: 1 addition & 0 deletions build-system/test-configs/forbidden-terms.js
Expand Up @@ -644,6 +644,7 @@ const forbiddenTermsGlobal = {
'src/experiments/shame.extern.js',
'src/mode.js',
'src/core/mode/test.js',
'src/core/mode/local-dev.js',
'src/web-worker/web-worker.js', // Web worker custom error reporter.
'testing/init-tests.js',
'tools/experiments/experiments.js',
Expand Down
2 changes: 1 addition & 1 deletion src/core/context/node.js
Expand Up @@ -49,7 +49,7 @@ export class ContextNode {
let contextNode = /** @type {!ContextNode|undefined} */ (node[NODE_PROP]);
if (!contextNode) {
contextNode = new ContextNode(node, null);
if (mode.isTest()) {
if (mode.isLocalDev() || mode.isTest()) {
// The `Object.defineProperty({enumerable: false})` helps tests, but
// hurts performance. So this is only done in a dev/test modes.
Object.defineProperty(node, NODE_PROP, {
Expand Down
2 changes: 1 addition & 1 deletion src/core/dom/layout/viewport-observer.js
Expand Up @@ -43,7 +43,7 @@ const viewportCallbacks = new WeakMap();
*/
export function observeWithSharedInOb(element, viewportCallback) {
// There should never be two unique observers of the same element.
if (mode.isTest()) {
if (mode.isLocalDev()) {
devAssert(
!viewportCallbacks.has(element) ||
viewportCallbacks.get(element) === viewportCallback
Expand Down
1 change: 1 addition & 0 deletions src/core/mode/index.js
@@ -1,4 +1,5 @@
export {isProd} from './prod';
export {isLocalDev} from './local-dev';
export {isMinified} from './minified';
export {isTest} from './test';
export {version} from './version';
Expand Down
17 changes: 17 additions & 0 deletions src/core/mode/local-dev.js
@@ -0,0 +1,17 @@
import {isProd} from './prod';
import {isTest} from './test';

/**
* Returns true if executing in a local development or testing environment.
* Calls may be DCE'd when compiled based on isForDistribution and isTest.
*
* @param {!Window=} opt_win
* @return {boolean}
*/
export function isLocalDev(opt_win) {
if (isProd()) {
return false;
}

return !!self.AMP_CONFIG?.localDev || isTest(opt_win);
}
4 changes: 1 addition & 3 deletions src/core/mode/test.js
Expand Up @@ -11,7 +11,5 @@ export function isTest(opt_win) {
return false;
}
const win = opt_win || self;
return !(
win.AMP_CONFIG?.test === false || win.AMP_CONFIG?.localDev === false
);
return !!(win.AMP_CONFIG?.test || win.__AMP_TEST || win['__karma__']);
}
2 changes: 1 addition & 1 deletion src/mode.js
Expand Up @@ -47,7 +47,7 @@ function getMode_(win) {
// flags. This improved DCE on the production file we deploy as the code
// paths for localhost/testing/development are eliminated.
return {
localDev: coreMode.isTest(win),
localDev: coreMode.isLocalDev(win),
development: isModeDevelopment(win, hashParams),
esm: coreMode.isEsm(),
test: coreMode.isTest(win),
Expand Down
4 changes: 2 additions & 2 deletions src/preact/base-element.js
Expand Up @@ -736,7 +736,7 @@ export class PreactBaseElement extends BaseElement {
* @private
*/
checkApiWrapper_(current) {
if (!mode.isTest()) {
if (!mode.isLocalDev()) {
return;
}
// Hack around https://github.com/preactjs/preact/issues/3084
Expand Down Expand Up @@ -854,7 +854,7 @@ PreactBaseElement['staticProps'] = undefined;
/**
* @protected {!Array<!ContextProp>}
*/
PreactBaseElement['useContexts'] = mode.isTest() ? Object.freeze([]) : [];
PreactBaseElement['useContexts'] = mode.isLocalDev() ? Object.freeze([]) : [];

/**
* Whether the component implements a loading protocol.
Expand Down
3 changes: 1 addition & 2 deletions src/service/extension-script.js
Expand Up @@ -134,12 +134,11 @@ export function createExtensionScript(win, extensionId, version) {
if (getMode(win).test && win.testLocation) {
loc = win.testLocation;
}

const scriptSrc = calculateExtensionScriptUrl(
loc,
extensionId,
version,
getMode(win).test
getMode(win).localDev
);
scriptElement.src = scriptSrc;
return scriptElement;
Expand Down
9 changes: 7 additions & 2 deletions src/video-iframe-integration.js
Expand Up @@ -7,6 +7,7 @@ import {dict} from '#core/types/object';
import {tryParseJson} from '#core/types/object/json';

import {getData, listen} from './event-helper';
import {getMode} from './mode';

/** @fileoverview Entry point for documents inside an <amp-video-iframe>. */

Expand Down Expand Up @@ -364,7 +365,9 @@ export class AmpVideoIntegration {
const id = this.callCounter_++;
const completeData = {id, ...data};

this.win_?.parent./*OK*/ postMessage(completeData, '*');
if (!getMode(this.win_).test && this.win_.parent) {
this.win_.parent./*OK*/ postMessage(completeData, '*');
}

if (opt_callback) {
this.callbacks_[id] = opt_callback;
Expand Down Expand Up @@ -448,4 +451,6 @@ export function adopt(global) {
callbacks.forEach(callbacks.push);
}

adopt(self);
if (!getMode(self).test) {
adopt(self);
}
1 change: 0 additions & 1 deletion test/unit/core/window/test-history.js
Expand Up @@ -327,7 +327,6 @@ describes.sandboxed('Window - History', {}, (env) => {
});

it('should create natural binding and make it singleton', () => {
win.AMP_CONFIG = {test: false};
const history = Services.historyForDoc(ampdoc);
expect(history.binding_).to.be.instanceOf(HistoryBindingNatural_);
expect(win.__AMP_SERVICES.history.obj).to.equal(history);
Expand Down
8 changes: 3 additions & 5 deletions test/unit/test-runtime.js
Expand Up @@ -415,11 +415,9 @@ describes.fakeWin(
});

it('should load correct extension version', async function () {
// calculateExtensionScript isn't passed an opt_win arg
// so we can't use win.__AMP_MODE for both.
self.__AMP_MODE = {rtvVersion: 'test-version'};
win.__AMP_MODE = {test: false};

self.__AMP_MODE = {
rtvVersion: 'test-version',
};
function addExisting(index) {
const s = document.createElement('script');
const name = 'amp-test-element' + index;
Expand Down
4 changes: 2 additions & 2 deletions test/unit/test-viewport.js
Expand Up @@ -22,6 +22,7 @@ import {installVsyncService} from '#service/vsync-impl';

import {loadPromise} from '../../src/event-helper';
import {dev} from '../../src/log';
import {getMode} from '../../src/mode';
import {setParentWindow} from '../../src/service-helpers';

const NOOP = () => {};
Expand Down Expand Up @@ -1806,7 +1807,6 @@ describes.sandboxed('createViewport', {}, () => {
});

it('should NOT bind to "iOS embed" when iframed but not embedded', () => {
win.AMP_CONFIG = {test: false};
win.parent = {};
env.sandbox.stub(viewer, 'isEmbedded').callsFake(() => false);
installViewportServiceForDoc(ampDoc);
Expand All @@ -1816,6 +1816,7 @@ describes.sandboxed('createViewport', {}, () => {

it('should bind to "iOS embed" when iframed but in test mode', () => {
win.parent = {};
getMode(win).test = true;
env.sandbox.stub(viewer, 'isEmbedded').callsFake(() => false);
installViewportServiceForDoc(ampDoc);
const viewport = Services.viewportForDoc(ampDoc);
Expand All @@ -1825,7 +1826,6 @@ describes.sandboxed('createViewport', {}, () => {
});

it('should bind to "natural" when iframed, but iOS supports scrollable iframes', () => {
win.AMP_CONFIG = {test: false};
win.parent = {};
env.sandbox.stub(viewer, 'isEmbedded').callsFake(() => true);
env.sandbox
Expand Down

0 comments on commit 1318234

Please sign in to comment.