Skip to content

Cookie storage added as option #28

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
b10z wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Cookie storage added as option #28

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c4ee294
Cookie storage added as option
Sep 5, 2021
7252cf1
Revert "Cookie storage added as option"
Sep 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/Experiment.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import PropTypes from 'prop-types';
import CoreExperiment from './CoreExperiment';
import emitter from './emitter';
import store from './store';
import storeCookie from './storeCookie';

emitter.addActiveVariantListener(function (
experimentName,
Expand All @@ -12,7 +13,11 @@ emitter.addActiveVariantListener(function (
if (skipSave) {
return;
}
store.setItem('PUSHTELL-' + experimentName, variantName);
if (emitter.withCookie()) {
Copy link
Copy Markdown
Member

@moretti moretti Sep 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey this looks great, thanks for submitting this PR!

Justing thinking: I'm looking at https://caniuse.com/?search=localstorage
and it seems that window.localStorage is supported by almost any modern browser (96.6% of the users at the moment).

So maybe rather than manually setting this option via emitter we could extend the existing store to fallback to cookies when localStorage is not supported.

For example something like:

import Cookies from 'js-cookie';

// https://github.com/Modernizr/Modernizr/blob/d4c7b6082709e32fb0589ba38aa96581d44ce395/feature-detects/storage/cookies.js#L16-L35
const cookiesEnabled = (() => {
  try {
    document.cookie = 'cookietest=1';
    const ret = document.cookie.indexOf('cookietest=') !== -1;
    document.cookie = 'cookietest=1; expires=Thu, 01-Jan-1970 00:00:01 GMT';
    return ret;
  } catch (e) {
    return false;
  }
})();

// https://github.com/Modernizr/Modernizr/blob/d4c7b6082709e32fb0589ba38aa96581d44ce395/feature-detects/storage/localstorage.js#L17-L46
const localStorageEnabled = (() => {
  const mod = 'modernizr';
  try {
    localStorage.setItem(mod, mod);
    localStorage.removeItem(mod);
    return true;
  } catch (e) {
    return false;
  }
})();

const store = {
    getItem(key) {
        if (localStorageEnabled) {
            return localStorage.getItem(key);
        }
        if (cookiesEnabled) {
            return Cookies.get(key);
        }
    }, 
    setItem(key, value) {
        if (localStorageEnabled) {
            return localStorage.setItem(key, value);
        }
        if (cookiesEnabled) {
            return Cookies.set(key, value);
        }
    }
};

export default store;

could replace the existing store and be more resilient? 🤔
This includes some snippets from Modernizr to perform feature detection and use js-cookie to read/write cookies. The library is only 800 bytes, so it should be fine to add it as dependency.
What do you think?

Copy link
Copy Markdown
Author

@b10z b10z Sep 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey,
Thanks for the comment! You are right, this might be the correct way of implementing the feature. Also, I believe that the code you provided is nicely written and elegant. Have you tested the code above with the tool? If you haven't, I have no problem of doing it and provide any changes needed!

Copy link
Copy Markdown
Author

@b10z b10z Sep 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey!
I added the provided code, reverted my previous commit and installed js-cookie as well. The code tested on both of my dummy react projects, everything worked as expected, even when i blocked local storage the code created a cookie as expected. For any change/fix/test/whatever please let me know.

Copy link
Copy Markdown
Member

@moretti moretti Sep 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @b10z sorry for the late reply, unfortunately I didn't have much free time recently, but I'll have a look next week. Thanks again for contributing to the project!

storeCookie().setCookie('PUSHTELL_COOKIE-' + experimentName, variantName);
} else {
store.setItem('PUSHTELL-' + experimentName, variantName);
}
});

export default class Experiment extends Component {
Expand Down
9 changes: 8 additions & 1 deletion src/calculateActiveVariant.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import crc32 from 'fbjs/lib/crc32';
import emitter from './emitter';
import store from './store';
import storeCookie from './storeCookie';

const calculateVariant = (experimentName, userIdentifier) => {
/*
Expand Down Expand Up @@ -57,11 +58,17 @@ export default (experimentName, userIdentifier, defaultVariantName) => {
if (typeof activeValue === 'string') {
return activeValue;
}
const storedValue = store.getItem('PUSHTELL-' + experimentName);
let storedValue;
if (emitter.withCookie()) {
storedValue = storeCookie().getCookie('PUSHTELL_COOKIE-' + experimentName);
} else {
storedValue = store.getItem('PUSHTELL-' + experimentName);
}
if (typeof storedValue === 'string') {
emitter.setActiveVariant(experimentName, storedValue, true);
return storedValue;
}

if (typeof defaultVariantName === 'string') {
emitter.setActiveVariant(experimentName, defaultVariantName);
return defaultVariantName;
Expand Down
11 changes: 11 additions & 0 deletions src/emitter.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ let activeExperiments = {};
let experimentsWithDefinedVariants = {};
let playedExperiments = {};
let customDistributionAlgorithm = undefined;
let enableCookies = false;

const emitter = new EventEmitter();

Expand Down Expand Up @@ -279,4 +280,14 @@ PushtellEventEmitter.prototype.addExperimentVariant = function (
experiments[experimentName][variantName] = true;
};

PushtellEventEmitter.prototype.setCookie = function (enabled) {
if (typeof enabled == 'boolean') {
enableCookies = enabled;
}
};

PushtellEventEmitter.prototype.withCookie = function () {
return enableCookies;
};

export default new PushtellEventEmitter();
28 changes: 28 additions & 0 deletions src/storeCookie.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
const storeCookie = () => {
const cookieStorage = {
getCookie: function (c_name) {
let c_value = document.cookie,
c_start = c_value.indexOf(' ' + c_name + '=');
if (c_start == -1) c_start = c_value.indexOf(c_name + '=');
if (c_start == -1) {
c_value = null;
} else {
c_start = c_value.indexOf('=', c_start) + 1;
var c_end = c_value.indexOf(';', c_start);
if (c_end == -1) {
c_end = c_value.length;
}
c_value = unescape(c_value.substring(c_start, c_end));
}
return c_value;
},
setCookie: function (c_name, variantName) {
if (!this.getCookie(c_name)) {
document.cookie = c_name + '=' + variantName;
}
},
};
return cookieStorage;
};

export default storeCookie;