Skip to content

Append this._storageSuffix (which includes API key) to unsentKey #204

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

Merged
merged 4 commits into from
Nov 23, 2019
Merged

Append this._storageSuffix (which includes API key) to unsentKey #204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7fb0749
Append this._storageSuffix (which includes API key) to unsentKey befo…
sputh Nov 7, 2019
e99863f
Append storageSuffix to unsentIdentifyKeys as well, add migration to …
sputh Nov 15, 2019
86479c4
Fix Tests. TODO: add tests for react native
sputh Nov 18, 2019
fe025f0
Bump Version v5.7.0
sputh Nov 23, 2019
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
### 5.7.0 (November 22, 2019)
* Namespace AsyncStorage with api key to prevent cross domain contamination

### 5.6.0 (October 21, 2019)

* Drop esm module from package.json to prevent it from being the default build.
Expand Down
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ Please see our [installation guide](https://amplitude.zendesk.com/hc/en-us/artic
[![npm version](https://badge.fury.io/js/amplitude-js.svg)](https://badge.fury.io/js/amplitude-js)
[![Bower version](https://badge.fury.io/bo/amplitude-js.svg)](https://badge.fury.io/bo/amplitude-js)

[5.6.0 - Released on October 21, 2019](https://github.com/amplitude/Amplitude-JavaScript/releases/latest)
[5.7.0 - Released on November 22, 2019](https://github.com/amplitude/Amplitude-JavaScript/releases/latest)


# JavaScript SDK Reference #
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "amplitude-js",
"author": "Amplitude <support@amplitude.com>",
"version": "5.6.0",
"version": "5.7.0",
"license": "MIT",
"description": "Javascript library for Amplitude Analytics",
"keywords": [
Expand Down
112 changes: 71 additions & 41 deletions src/amplitude-client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,47 +166,49 @@ AmplitudeClient.prototype.init = function init(apiKey, opt_userId, opt_config, o
};

if (AsyncStorage) {
Promise.all([
AsyncStorage.getItem(this._storageSuffix),
AsyncStorage.getItem(this.options.unsentKey),
AsyncStorage.getItem(this.options.unsentIdentifyKey),
]).then((values) => {
if (values[0]) {
const cookieData = JSON.parse(values[0]);
if (cookieData) {
_loadCookieDataProps(this, cookieData);
this._migrateUnsentEvents(() => {
Promise.all([
AsyncStorage.getItem(this._storageSuffix),
AsyncStorage.getItem(this.options.unsentKey + this._storageSuffix),
AsyncStorage.getItem(this.options.unsentIdentifyKey + this._storageSuffix),
]).then((values) => {
if (values[0]) {
const cookieData = JSON.parse(values[0]);
if (cookieData) {
_loadCookieDataProps(this, cookieData);
}
}
}
if (this.options.saveEvents) {
this._unsentEvents = this._parseSavedUnsentEventsString(values[1]).concat(this._unsentEvents);
this._unsentIdentifys = this._parseSavedUnsentEventsString(values[2]).concat(this._unsentIdentifys);
}
if (DeviceInfo) {
Promise.all([
DeviceInfo.getCarrier(),
DeviceInfo.getModel(),
DeviceInfo.getManufacturer(),
DeviceInfo.getUniqueId(),
]).then(values => {
this.deviceInfo = {
carrier: values[0],
model: values[1],
manufacturer: values[2]
};
initFromStorage(values[3]);
if (this.options.saveEvents) {
this._unsentEvents = this._parseSavedUnsentEventsString(values[1]).concat(this._unsentEvents);
this._unsentIdentifys = this._parseSavedUnsentEventsString(values[2]).concat(this._unsentIdentifys);
}
if (DeviceInfo) {
Promise.all([
DeviceInfo.getCarrier(),
DeviceInfo.getModel(),
DeviceInfo.getManufacturer(),
DeviceInfo.getUniqueId(),
]).then(values => {
this.deviceInfo = {
carrier: values[0],
model: values[1],
manufacturer: values[2]
};
initFromStorage(values[3]);
this.runQueuedFunctions();
if (type(opt_callback) === 'function') {
opt_callback(this);
}
}).catch((err) => {
this.options.onError(err);
});
} else {
initFromStorage();
this.runQueuedFunctions();
if (type(opt_callback) === 'function') {
opt_callback(this);
}
}).catch((err) => {
this.options.onError(err);
});
} else {
initFromStorage();
this.runQueuedFunctions();
}
}).catch((err) => {
this.options.onError(err);
}
}).catch((err) => {
this.options.onError(err);
});
Copy link
Contributor Author

@sputh sputh Nov 15, 2019

Choose a reason for hiding this comment

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

this is all wrapped in the callback of the _migrateUnsentEvents

});
} else {
if (this.options.saveEvents) {
Expand All @@ -225,6 +227,34 @@ AmplitudeClient.prototype.init = function init(apiKey, opt_userId, opt_config, o
}
};

/**
* @private
*/
AmplitudeClient.prototype._migrateUnsentEvents = function _migrateUnsentEvents(cb) {
Copy link
Member

@djih djih Nov 18, 2019

Choose a reason for hiding this comment

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

I'm curious why we have this callback chaining pattern in init? Is it because we need it to interact with AsyncStorage? in the case where init is just a consecutive / synchronous you could just write the logic all straight out without any callbacks.

Copy link
Contributor Author

@sputh sputh Nov 18, 2019

Choose a reason for hiding this comment

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

to make sure they're not touching asyncStorage at the same time, e.g. making it synchronous via migrating unsent events before accessing it

Promise.all([
AsyncStorage.getItem(this.options.unsentKey),
AsyncStorage.getItem(this.options.unsentIdentifyKey),
]).then((values) => {
Copy link
Member

@djih djih Nov 18, 2019

Choose a reason for hiding this comment

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

Are you running this migration every single time the SDK inits? You only need to run it one time, which is when the user loads this new SDK for the first time. Do you need to check if values is null or something for all the subsequent times it's run?? You could also just check that the unsentKey and unsentIdentifyKey exist in Async storage, if not just skip this entire code block?

if (this.options.saveEvents) {
var unsentEventsString = values[0];
var unsentIdentifyKey = values[1];
Promise.all([
AsyncStorage.setItem(this.options.unsentKey + this._storageSuffix, unsentEventsString),
AsyncStorage.setItem(this.options.unsentIdentifyKey + this._storageSuffix, unsentIdentifyKey),
]).then(() => {
Promise.all([
AsyncStorage.removeItem(this.options.unsentKey),
AsyncStorage.removeItem(this.options.unsentIdentifyKey),
]).then(cb);
}).catch((err) => {
this.options.onError(err);
});
}
}).catch((err) => {
this.options.onError(err);
});
};

/**
* @private
*/
Expand Down Expand Up @@ -702,15 +732,15 @@ AmplitudeClient.prototype._saveReferrer = function _saveReferrer(referrer) {
AmplitudeClient.prototype.saveEvents = function saveEvents() {
try {
if (AsyncStorage) {
AsyncStorage.setItem(this.options.unsentKey, JSON.stringify(this._unsentEvents));
AsyncStorage.setItem(this.options.unsentKey + this._storageSuffix, JSON.stringify(this._unsentEvents));
Copy link
Member

@djih djih Nov 18, 2019

Choose a reason for hiding this comment

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

not blocking but in terms of API I feel like _setInStorage should encapsulate the AsyncStorage logic inside it, so that whoever calls _setInStorage doesn't have to worry about checking Async etc etc you know what I mean?

} else {
this._setInStorage(localStorage, this.options.unsentKey, JSON.stringify(this._unsentEvents));
}
} catch (e) {}

try {
if (AsyncStorage) {
AsyncStorage.setItem(this.options.unsentIdentifyKey, JSON.stringify(this._unsentIdentifys));
AsyncStorage.setItem(this.options.unsentIdentifyKey + this._storageSuffix, JSON.stringify(this._unsentIdentifys));
} else {
this._setInStorage(localStorage, this.options.unsentIdentifyKey, JSON.stringify(this._unsentIdentifys));
}
Expand Down
4 changes: 2 additions & 2 deletions src/amplitude-snippet.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@
var amplitude = window.amplitude || {'_q':[],'_iq':{}};
var as = document.createElement('script');
as.type = 'text/javascript';
as.integrity = 'sha384-t5vT47el2d0e6uQ1h75P9Lbzo8by6pbk+Rg41Gm4xuTGR+eDLpbWslKUtZMDe9Bj';
as.integrity = 'sha384-rSEVPt+HsYVwBs0EY4dB3fOcSZOW9cbAQV2CqsLFDjNbdiNyoXcGruquK0IyWxAZ';
as.crossOrigin = 'anonymous';
as.async = true;
as.src = 'https://cdn.amplitude.com/libs/amplitude-5.6.0-min.gz.js';
as.src = 'https://cdn.amplitude.com/libs/amplitude-5.7.0-min.gz.js';
as.onload = function() {if(!window.amplitude.runQueuedFunctions) {console.log('[Amplitude] Error: could not load SDK');}};
var s = document.getElementsByTagName('script')[0];
s.parentNode.insertBefore(as, s);
Expand Down