Skip to content
Closed
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
246 changes: 199 additions & 47 deletions src/__tests__/browserSuites/ready-from-cache.spec.js

Large diffs are not rendered by default.

44 changes: 44 additions & 0 deletions src/__tests__/mocks/preloadedData.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
export const splitDefinitions = {
p1__split: {
'name': 'p1__split',
'status': 'ACTIVE',
'conditions': []
},
p2__split: {
'name': 'p2__split',
'status': 'ACTIVE',
'conditions': []
},
p3__split: {
'name': 'p3__split',
'status': 'ACTIVE',
'conditions': []
},
};

export const splitSerializedDefinitions = (function () {
return Object.keys(splitDefinitions).reduce((acum, splitName) => {
acum[splitName] = JSON.stringify(splitDefinitions[splitName]);
return acum;
}, {});
}());

export const segmentsDefinitions = {
segment_1: {
'name': 'segment_1',
'added': ['[email protected]'],
},
};

export const segmentsSerializedDefinitions = (function () {
return Object.keys(segmentsDefinitions).reduce((acum, segmentName) => {
acum[segmentName] = JSON.stringify(segmentsDefinitions[segmentName]);
return acum;
}, {});
}());

export const preloadedDataWithSegments = {
since: 1457552620999,
splitsData: splitSerializedDefinitions,
segmentsData: segmentsSerializedDefinitions
};
52 changes: 52 additions & 0 deletions src/storage/DataLoader.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/**
* Factory of data loaders
*
* @TODO update comment
* @param {Object} preloadedData validated data following the format proposed in https://github.com/godaddy/split-javascript-data-loader
* and extended with a `mySegmentsData` property.
*/
export function dataLoaderFactory(preloadedData = {}) {

/**
* Storage-agnostic adaptation of `loadDataIntoLocalStorage` function
* (https://github.com/godaddy/split-javascript-data-loader/blob/master/src/load-data.js)
*
* @param {Object} storage storage for client-side
* @param {Object} userId main user key defined at the SDK config
*/
return function loadData(storage, userId) {
// Do not load data if current preloadedData is empty
if (Object.keys(preloadedData).length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Be consistent with the 1 line if's on inputvalidation and remove the brackets. Or add them there, but choose one.

return;
}

const { segmentsData = {}, since = 0, splitsData = {} } = preloadedData;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should just use -1 for standardization with "that's the value for no cache" (specially given it'll be loaded on the storage) and change the condition in 28th, to be independent on this variable particular startup value, to if (currentSince > since) return


const currentSince = storage.splits.getChangeNumber();
Copy link
Contributor

Choose a reason for hiding this comment

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

storedSince might be more representative.


// Do not load data if current localStorage data is more recent
if (since <= currentSince) {
Copy link
Contributor

Choose a reason for hiding this comment

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

One liner if, standarize.

return;
}
// cleaning up the localStorage data, since some cached splits might need be part of the preloaded data
storage.splits.flush();
storage.splits.setChangeNumber(since);

// splitsData in an object where the property is the split name and the pertaining value is a stringified json of its data
Object.keys(splitsData).forEach(splitName => {
storage.splits.addSplit(splitName, splitsData[splitName]);
});

// add mySegments data
let userIdMySegmentsData = preloadedData.mySegmentsData && preloadedData.mySegmentsData[userId];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove the userIdMySegmentsData and either just have it mySegmentsData (shorter, concise, this has a client scope so we shouldn't need the clarification) or name it 'currentKeyMySegmentsData' or similar.

if (!userIdMySegmentsData) {
// segmentsData in an object where the property is the segment name and the pertaining value is a stringified object that contains the `added` array of userIds
userIdMySegmentsData = Object.keys(segmentsData).filter(segmentName => {
const added = JSON.parse(segmentsData[segmentName]).added;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rename added to represent what it stands for here?
Regardless, it's reaally likely that'll change, since there's no need to respect the BE json after we publish our serializer.

return added.indexOf(userId) > -1;
});
}
storage.segments.resetSegments(userIdMySegmentsData);
};

}
5 changes: 3 additions & 2 deletions src/storage/SplitCache/InMemory.js
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,11 @@ class SplitCacheInMemory {
}

/**
* Check if the splits information is already stored in cache. In memory there is no cache to check.
* Check if the splits information is already stored in cache. The data can be preloaded and passed via the config.
*/
checkCache() {
return false;
// @TODO rollback if we decide not to emit SDK_READY_FROM_CACHE using InMemory storage
return this.getChangeNumber() > -1;
}
}

Expand Down
14 changes: 12 additions & 2 deletions src/storage/browser.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,13 @@ export const DEFAULT_CACHE_EXPIRATION_IN_MILLIS = 864000000; // 10 days
const BrowserStorageFactory = context => {
const settings = context.get(context.constants.SETTINGS);
const { storage } = settings;
let result;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not "instance" ? It's not the result of an operation.. (it is but it isn't, I mean, it's not a list of values but an instance created by this factory)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

definitively right


switch (storage.type) {
case STORAGE_MEMORY: {
const keys = new KeyBuilder(settings);

return {
result = {
splits: new SplitCacheInMemory,
segments: new SegmentCacheInMemory(keys),
impressions: new ImpressionsCacheInMemory,
Expand Down Expand Up @@ -57,13 +58,14 @@ const BrowserStorageFactory = context => {
this.events.clear();
}
};
break;
}

case STORAGE_LOCALSTORAGE: {
const keys = new KeyBuilderLocalStorage(settings);
const expirationTimestamp = Date.now() - DEFAULT_CACHE_EXPIRATION_IN_MILLIS;

return {
result = {
splits: new SplitCacheInLocalStorage(keys, expirationTimestamp, settings.sync.__splitFiltersValidation),
segments: new SegmentCacheInLocalStorage(keys),
impressions: new ImpressionsCacheInMemory,
Expand Down Expand Up @@ -99,12 +101,20 @@ const BrowserStorageFactory = context => {
this.events.clear();
}
};
break;
}

default:
throw new Error('Unsupported storage type');
}

// load precached data into storage
if (storage.dataLoader) {
const key = settings.core.key;
storage.dataLoader(result, key);
}

return result;
};

export default BrowserStorageFactory;
56 changes: 56 additions & 0 deletions src/utils/inputValidation/preloadedData.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import { isObject, isString } from '../lang';
import logFactory from '../logger';
const log = logFactory('', {
displayAllErrors: true
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not so sure about this... Why is it true here? What's the reasoning behind it?

});

function validateSinceData(maybeSince, method) {
if (maybeSince > -1) return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Be careful. If you do maybeSince > -1 with undefined it yields false, but if you do it with null it yields true. The validation should check that's a number before anything, then you see how big it is.

Null is probably what you'll get if the BE serializer can't assign a proper one or there's some sort of parsing error somewhere (remember the data serializer won't be running in this same app), given that most languages won't send undefined.

log.error(`${method}: preloadedData.since must be a positive number.`);
return false;
}

function validateSplitsData(maybeSplitsData, method) {
if (isObject(maybeSplitsData)) {
const splitNames = Object.keys(maybeSplitsData);
if (splitNames.length > 0 && splitNames.every(splitName => isString(maybeSplitsData[splitName]))) return true;
}
log.error(`${method}: preloadedData.splitsData must be a map of split names to their serialized definitions.`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should make this a separate validation.
For an empty list we throw a warning (since it's possible to have an empty list of splits for an env).

For a list with items, if not all have the right format (keep isString for now or go and add the basic validation for Splits already, to check the key parts of the JSON) we'll log an error for invalid values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. It is a good idea to use the existing split validation utils

return false;
}

function validateMySegmentsData(maybeMySegmentsData, method) {
if (isObject(maybeMySegmentsData)) {
const userKeys = Object.keys(maybeMySegmentsData);
if (userKeys.length > 0 && userKeys.every(userKey => {
const segmentNames = maybeMySegmentsData[userKey];
// an empty list is valid
return Array.isArray(segmentNames) && segmentNames.every(segmentName => isString(segmentName));
})) return true;
}
log.error(`${method}: preloadedData.mySegmentsData must be a map of user keys to their list of segment names.`);
Copy link
Contributor

Choose a reason for hiding this comment

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

If list is empty it shouldn't be an error.

return false;
}

function validateSegmentsData(maybeSegmentsData, method) {
if (isObject(maybeSegmentsData)) {
const segmentNames = Object.keys(maybeSegmentsData);
if (segmentNames.length > 0 && segmentNames.every(segmentName => isString(maybeSegmentsData[segmentName]))) return true;
}
log.error(`${method}: preloadedData.segmentsData must be a map of segment names to their serialized definitions.`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, if list is empty it shouldn't be an error.

return false;
}

export function validatePreloadedData(maybePreloadedData, method) {
if (!isObject(maybePreloadedData)) {
log.error(`${method}: preloadedData must be an object.`);
} else if (
validateSinceData(maybePreloadedData.since, method) &&
validateSplitsData(maybePreloadedData.splitsData, method) &&
(!maybePreloadedData.mySegmentsData || validateMySegmentsData(maybePreloadedData.mySegmentsData, method)) &&
(!maybePreloadedData.segmentsData || validateSegmentsData(maybePreloadedData.segmentsData, method))
) {
return true;
}
return false;
}
2 changes: 1 addition & 1 deletion src/utils/logger/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ const initialState = String(
localStorage.getItem(LS_KEY) : ''
);

const createLog = (namespace, options = {}) => new Logger(namespace, merge(options, defaultOptions));
const createLog = (namespace, options = {}) => new Logger(namespace, merge(defaultOptions, options));
Copy link
Contributor

@NicoZelaya NicoZelaya Aug 20, 2020

Choose a reason for hiding this comment

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

Ain't this overriding the default options? We are assigning to a new variable in the merge method but objects are assigned per reference, not a copy.

Are you sure of this? The fact that all errors were hidden and the log level is shown is enforced on purpose. We want a centralized knob to tweak ALL loggers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right. I will rollback this change, and recheck it in another moment.


const ownLog = createLog('splitio-utils:logger');

Expand Down
10 changes: 7 additions & 3 deletions src/utils/settings/storage/browser.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,17 @@ import {
STORAGE_MEMORY,
STORAGE_LOCALSTORAGE
} from '../../../utils/constants';
import { validatePreloadedData } from '../../inputValidation/preloadedData';
import { dataLoaderFactory } from '../../../storage/DataLoader';

const ParseStorageSettings = settings => {
let {
mode,
storage: {
type = STORAGE_MEMORY,
options = {},
prefix
prefix,
preloadedData
},
} = settings;

Expand All @@ -47,15 +50,16 @@ const ParseStorageSettings = settings => {
// If an invalid storage type is provided OR we want to use LOCALSTORAGE and
// it's not available, fallback into MEMORY
if (type !== STORAGE_MEMORY && type !== STORAGE_LOCALSTORAGE ||
type === STORAGE_LOCALSTORAGE && !isLocalStorageAvailable()) {
type === STORAGE_LOCALSTORAGE && !isLocalStorageAvailable()) {
type = STORAGE_MEMORY;
log.warn('Invalid or unavailable storage. Fallbacking into MEMORY storage');
}

return {
type,
options,
prefix
prefix,
dataLoader: validatePreloadedData(preloadedData, 'Factory instantiation - storage preload') ? dataLoaderFactory(preloadedData) : undefined
};
};

Expand Down
18 changes: 17 additions & 1 deletion ts-tests/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,21 @@ impressionListener.logImpression(impressionData);
// Split filters
let splitFilters: SplitIO.SplitFilter[] = [{ type: 'byName', values: ['my_split_1', 'my_split_1'] }, { type: 'byPrefix', values: ['my_split', 'test_split_'] }]

// storage preloaded data
let preloadedData: SplitIO.PreloadedData = {
since: 10000,
splitsData: {
split_1: 'SPLIT_1_DEFINITION', split_2: 'SPLIT_2_DEFINITION'
},
mySegmentsData: {
user_id_1: [],
user_id_2: ['segment_1', 'segment_2']
},
segmentsData: {
segment_1: 'SEGMENT_1_DEFINITION', segment_2: 'SEGMENT_2_DEFINITION'
}
}

// Browser integrations
let fieldsObjectSample: UniversalAnalytics.FieldsObject = { hitType: 'event', eventAction: 'action' };
let eventDataSample: SplitIO.EventData = { eventTypeId: 'someEventTypeId', value: 10, properties: {} }
Expand Down Expand Up @@ -442,7 +457,8 @@ let fullBrowserSettings: SplitIO.IBrowserSettings = {
features: mockedFeaturesMap,
storage: {
type: 'LOCALSTORAGE',
prefix: 'PREFIX'
prefix: 'PREFIX',
preloadedData: preloadedData
},
impressionListener: impressionListener,
debug: true,
Expand Down
38 changes: 37 additions & 1 deletion types/splitio.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -750,6 +750,35 @@ declare namespace SplitIO {
*/
values: string[],
}
/**
* Defines the format of Split data to preload on the factory storage (cache).
*/
interface PreloadedData {
/**
* Change number of the preloaded data.
* If this value is older than the current changeNumber at the storage, the data is not used to update the storage content.
*/
since: number,
/**
* Map of splits to their serialized definitions.
*/
splitsData: {
[splitName: string]: string
},
/**
* Optional map of user keys to their list of segments.
*/
mySegmentsData?: {
[key: string]: string[]
},
/**
* Optional map of segments to their serialized definitions.
* This property is ignored if `mySegmentsData` was provided.
*/
segmentsData?: {
[segmentName: string]: string
},
}
/**
* Settings interface for SDK instances created on the browser
* @interface IBrowserSettings
Expand Down Expand Up @@ -902,7 +931,14 @@ declare namespace SplitIO {
* @property {string} prefix
* @default SPLITIO
*/
prefix?: string
prefix?: string,
/**
* Split data to preload the storage. You may optionally specify it to quickly initialice and use the SDK with cached data.
* If the data is valid, the SDK emits an SDK_READY_FROM_CACHE event once it is ready to be used.
* @see {@link https://help.split.io/hc/en-us/articles/360020448791-JavaScript-SDK#preloaded-data}
* @property {PreloadedData} preloadedData
*/
preloadedData?: PreloadedData,
}
/**
* SDK integration settings for the Browser.
Expand Down