Data loader [WIP] #467
Data loader [WIP] #467
Conversation
src/storage/DataLoader.js
Outdated
| return; | ||
| } | ||
|
|
||
| const { segmentsData = {}, since = 0, splitsData = {} } = preloadedData; |
There was a problem hiding this comment.
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
src/storage/DataLoader.js
Outdated
|
|
||
| const { segmentsData = {}, since = 0, splitsData = {} } = preloadedData; | ||
|
|
||
| const currentSince = storage.splits.getChangeNumber(); |
There was a problem hiding this comment.
storedSince might be more representative.
src/storage/DataLoader.js
Outdated
| 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; |
There was a problem hiding this comment.
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.
src/storage/DataLoader.js
Outdated
| }); | ||
|
|
||
| // add mySegments data | ||
| let userIdMySegmentsData = preloadedData.mySegmentsData && preloadedData.mySegmentsData[userId]; |
There was a problem hiding this comment.
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.
src/storage/browser.js
Outdated
| @@ -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; | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
definitively right
| }); | ||
|
|
||
| function validateSinceData(maybeSince, method) { | ||
| if (maybeSince > -1) return true; |
There was a problem hiding this comment.
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.
| 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.`); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks. It is a good idea to use the existing split validation utils
| 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.`); |
There was a problem hiding this comment.
Same, if list is empty it shouldn't be an error.
| 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.`); |
There was a problem hiding this comment.
If list is empty it shouldn't be an error.
src/utils/logger/index.js
Outdated
| @@ -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)); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
you are right. I will rollback this change, and recheck it in another moment.
|
SonarQube Quality Gate failed. |
|
SonarQube Quality Gate failed. |
JS SDK
What did you accomplish?
How do we test the changes introduced in this PR?
Extra Notes