Skip to content

Commit

Permalink
Refactor experiments to use localStorage (ampproject#22796)
Browse files Browse the repository at this point in the history
* Refactor experiments to use localStorage, not cookies.

* Update experiments.html.

* Fix lint.

* Whitelist in presubmit check for localStorage.

* Show redirect tool on cdn.ampproject.org instead of experiment toggles.

* Use util functions where possible.

* Add user warning to AMP.toggleExperiment.

* Update experiments/README.md.

* Fix tests.

* Try/catch localStorage, support origin URLs.

* Remove viewer URL support.

* Move localStorage check inside try/catch.
  • Loading branch information
William Chou authored and RINDO committed Jul 24, 2019
1 parent ecf3a70 commit bd80446
Show file tree
Hide file tree
Showing 7 changed files with 295 additions and 202 deletions.
16 changes: 9 additions & 7 deletions build-system/tasks/presubmit-checks.js
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,7 @@ const forbiddenTerms = {
'localStorage': {
message: requiresReviewPrivacy,
whitelist: [
'src/experiments.js',
'src/service/cid-impl.js',
'src/service/storage-impl.js',
'testing/fake-dom.js',
Expand Down Expand Up @@ -923,23 +924,24 @@ const forbiddenTermsSrcInclusive = {
'code. Use a property of urls from src/config.js instead.',
whitelist: [
'ads/_a4a-config.js',
'build-system/app.js',
'build-system/app-index/amphtml-helpers.js',
'build-system/amp4test.js',
'build-system/app-index/amphtml-helpers.js',
'build-system/app-video-testbench.js',
'build-system/app.js',
'build-system/shadow-viewer.js',
'build-system/tasks/check-links.js',
'build-system/tasks/extension-generator/index.js',
'build-system/tasks/helpers.js',
'dist.3p/current/integration.js',
'extensions/amp-iframe/0.1/amp-iframe.js',
'src/3p-frame.js',
'src/config.js',
'testing/local-amp-chrome-extension/background.js',
'tools/errortracker/errortracker.go',
'tools/experiments/experiments.js',
'validator/engine/validator-in-browser.js',
'validator/nodejs/index.js',
'validator/webui/serve-standalone.go',
'build-system/app-video-testbench.js',
'build-system/shadow-viewer.js',
'build-system/tasks/check-links.js',
'build-system/tasks/extension-generator/index.js',
'build-system/tasks/helpers.js',
],
},
'\\<\\<\\<\\<\\<\\<': {
Expand Down
79 changes: 43 additions & 36 deletions src/experiments.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,16 @@
* Experiments page: https://cdn.ampproject.org/experiments.html *
*/

import {getCookie, setCookie} from './cookies';
import {dev, user} from './log';
import {getMode} from './mode';
import {hasOwn} from './utils/object';
import {parseQueryString} from './url';

/** @const {string} */
const COOKIE_NAME = 'AMP_EXP';
const TAG = 'EXPERIMENTS';

/** @const {number} */
const COOKIE_MAX_AGE_DAYS = 180; // 6 month

/** @const {time} */
const COOKIE_EXPIRATION_INTERVAL = COOKIE_MAX_AGE_DAYS * 24 * 60 * 60 * 1000;
/** @const {string} */
const LOCAL_STORAGE_KEY = 'amp-experiment-toggles';

/** @const {string} */
const TOGGLES_WINDOW_PROPERTY = '__AMP__EXPERIMENT_TOGGLES';
Expand Down Expand Up @@ -84,7 +82,7 @@ export function isExperimentOn(win, experimentId) {
* @param {boolean=} opt_on
* @param {boolean=} opt_transientExperiment Whether to toggle the
* experiment state "transiently" (i.e., for this page load only) or
* durably (by saving the experiment IDs to the cookie after toggling).
* durably (by saving the experiment IDs after toggling).
* Default: false (save durably).
* @return {boolean} New state for experimentId.
*/
Expand All @@ -101,17 +99,27 @@ export function toggleExperiment(
toggles[experimentId] = on;

if (!opt_transientExperiment) {
const cookieToggles = getExperimentTogglesFromCookie(win);
cookieToggles[experimentId] = on;
saveExperimentTogglesToCookie(win, cookieToggles);
const storedToggles = getExperimentToggles(win);
storedToggles[experimentId] = on;
saveExperimentToggles(win, storedToggles);
// Avoid affecting tests that spy/stub warn().
if (!getMode().test) {
user().warn(
TAG,
'"%s" experiment %s for the domain "%s". See: https://amp.dev/documentation/guides-and-tutorials/learn/experimental',
experimentId,
on ? 'enabled' : 'disabled',
win.location.hostname
);
}
}
}
return on;
}

/**
* Calculate whether the experiment is on or off based off of the
* cookieFlag or the global config frequency given.
* Calculate whether the experiment is on or off based off of its default value,
* stored overriden value, or the global config frequency given.
* @param {!Window} win
* @return {!Object<string, boolean>}
*/
Expand Down Expand Up @@ -151,7 +159,7 @@ export function experimentToggles(win) {
}
}

Object.assign(toggles, getExperimentTogglesFromCookie(win));
Object.assign(toggles, getExperimentToggles(win));

if (
win.AMP_CONFIG &&
Expand Down Expand Up @@ -189,9 +197,16 @@ export function experimentTogglesOrNull(win) {
* @param {!Window} win
* @return {!Object<string, boolean>}
*/
function getExperimentTogglesFromCookie(win) {
const experimentCookie = getCookie(win, COOKIE_NAME);
const tokens = experimentCookie ? experimentCookie.split(/\s*,\s*/g) : [];
function getExperimentToggles(win) {
let experimentsString = '';
try {
if ('localStorage' in win) {
experimentsString = win.localStorage.getItem(LOCAL_STORAGE_KEY);
}
} catch (e) {
dev().expectedError(TAG, 'localStorage not supported.');
}
const tokens = experimentsString ? experimentsString.split(/\s*,\s*/g) : [];

const toggles = Object.create(null);
for (let i = 0; i < tokens.length; i++) {
Expand All @@ -204,7 +219,6 @@ function getExperimentTogglesFromCookie(win) {
toggles[tokens[i]] = true;
}
}

return toggles;
}

Expand All @@ -213,33 +227,28 @@ function getExperimentTogglesFromCookie(win) {
* @param {!Window} win
* @param {!Object<string, boolean>} toggles
*/
function saveExperimentTogglesToCookie(win, toggles) {
function saveExperimentToggles(win, toggles) {
const experimentIds = [];
for (const experiment in toggles) {
experimentIds.push((toggles[experiment] === false ? '-' : '') + experiment);
}

setCookie(
win,
COOKIE_NAME,
experimentIds.join(','),
Date.now() + COOKIE_EXPIRATION_INTERVAL,
{
// Set explicit domain, so the cookie gets send to sub domains.
domain: win.location.hostname,
allowOnProxyOrigin: true,
try {
if ('localStorage' in win) {
win.localStorage.setItem(LOCAL_STORAGE_KEY, experimentIds.join(','));
}
);
} catch (e) {
user().error(TAG, 'Failed to save experiments to localStorage.');
}
}

/**
* See getExperimentTogglesFromCookie().
* See getExperimentToggles().
* @param {!Window} win
* @return {!Object<string, boolean>}
* @visibleForTesting
*/
export function getExperimentToglesFromCookieForTesting(win) {
return getExperimentTogglesFromCookie(win);
export function getExperimentTogglesForTesting(win) {
return getExperimentToggles(win);
}

/**
Expand All @@ -248,9 +257,7 @@ export function getExperimentToglesFromCookieForTesting(win) {
* @visibleForTesting
*/
export function resetExperimentTogglesForTesting(win) {
setCookie(win, COOKIE_NAME, '', 0, {
domain: win.location.hostname,
});
saveExperimentToggles(win, {});
win[TOGGLES_WINDOW_PROPERTY] = null;
}

Expand Down
3 changes: 2 additions & 1 deletion test/unit/test-cid.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,8 @@ describe('cid', () => {
storage[key] = value;
},
getItem: key => {
expect(key).to.equal('amp-cid');
// isExperimentOn() in the code paths causes "amp-experiment-toggles".
expect(['amp-cid', 'amp-experiment-toggles']).to.contain(key);
return storage[key];
},
},
Expand Down
Loading

0 comments on commit bd80446

Please sign in to comment.