Add feature to deferInitialization #214

sputh · 2019-12-04T23:26:57Z

This is especially useful when you don't want to track any analytics or drop any analytic cookies prior to asking the users to opt in to analytics.
I've added a configuration (deferInitialization) and an API (enableTracking) to manage this.
If deferInitialization is set to true AND a customer has yet to opt in, we will locally store their pending logs until they either navigate away or opt in.

Note: This is a little gross because I'm checking whether or not to defer calling the function in EVERY public function. I considered using js decorators but it would require refactoring this to use classes. I considered iterating over an array of public methods and wrapping them, but that makes debugging much more difficult in the future. Ultimately, I opted for readability.

This is especially useful when you don't want to track any analytics or drop any analytic cookies prior to asking the users to opt in to analytics. I've added a configuration (deferInitialization) and an API (enableTracking) to manage this. If deferInitialization is set to true AND a customer has yet to opt in, we will locally store their pending logs until they either navigate away or opt in.

sputh · 2019-12-04T23:28:28Z

test/amplitude-client.js

+      });
+    });
+
+    describe('upon to opting into analytics', function () {


note: fix english 🤦🏻‍♀️

haoliu-amp · 2019-12-05T19:13:18Z

Look good to me, but would leave approval decision to Dan or Krishna.

djih

I think we need to discuss the API and how the customer might interact with this. It looks like you're requiring the customer to figure out if the user has opted in or out every single time on page load, and call enable tracking?

Does it make sense for us to track whether a user has opted in? Once the user opts in, we can skip all the defer initialization logic too. I feel like we should discuss this API and the customer developer interaction

A side note: I feel like adding if (this._shouldDeferCall()) { return this._q.push(['setDomain', domain]); }
is fragile. If one of us has a typo in the function name, or forgets one of the args, it will break. I wonder if there's a way to programatically do it, like how the snippet does arguments.slice to grab all the arguments: https://github.com/amplitude/Amplitude-JavaScript/blob/master/src/amplitude-snippet.js#L33

djih · 2019-12-05T20:55:08Z

src/amplitude-client.js

+ */
+AmplitudeClient.prototype._deferInitialization = function _deferInitialization(apiKey, opt_userId, opt_config, opt_callback) {
+  this._initializationDeferred = true;
+  this._q.push(['init', apiKey, opt_userId, opt_config, opt_callback]);


is this _q already used for something else? Are there any potential risks for re-using it and having the two different use cases collide? I feel like it's safer to use a separate queue for this.

it doesn't seem like there's any potential risk to re-using it. I think it would add more confusion if we had multiple queues that are vaguely defined

I hope this doesn't cause some weird circular queueing behavior where you defer a function that pushes to this or something, etc 🤔 (seems fine I guess?)

i can't imagine it having circular dependency issues 🤔
since it's a queue, it will run functions in order and it can tolerate if we pass in a function multiple times

src/amplitude-client.js

djih · 2019-12-05T21:04:48Z

src/amplitude-client.js

  }

+  var hasExistingCookie = !!this.cookieStorage.get(this.options.cookieName + this._storageSuffix);
+  if (opt_config && opt_config.deferInitialization && !hasExistingCookie) {


ok so let's say a user accepts and allows tracking and revisits the page the next day. There would already be an existing cookie and we would skip _deferInitialization. However, wouldn't this._shouldDeferCall() still return true?

No this._shouldDeferCall() would return false (for the most part, there's a short period of time in the init function where this._pendingReadStorage returns true while getting started).

It depends on how they revisit the page. If the page never refreshed and they have the same instance of Amplitude, then the new events will not be sent --- I've added a test for this

If it's a new instance of amplitude (e.g refresh page, navigate to another page), then hasExistingCookie will be true and _initializationDeferred will never be set to true, thus this._shouldDeferCall() will be false as long as this._pendingReadStorage is also false (most of the time)

After adding tests, and further inspecting, i realize you're right. Because _initializationDeferred is never set to false AND this._storageSuffix isn't defined until the try block below, it's always assuming _initializationDeferred is true.

I've added a test to reflect it and additional assert statements for the existing tests

djih

Also don't forget to add your new enable tracking method to the snippet

…g (when a user explicitly opts into analytics), move deferInitialization check where this._storageSuffix is actually defined (in the try block) * set initializedDeferred = false in enableTracking() * make sure this._storageSuffix is defined when cookies are being checked * Added tests for sending events after tracking is enabled * Added assertions to ensure that each test starts with a clean slate (no unsentEvents) before enabling tracking

sputh · 2019-12-05T22:53:39Z

src/amplitude-client.js

    this.options.apiKey = apiKey;
    this._storageSuffix = '_' + apiKey + this._legacyStorageSuffix;

+    var hasExistingCookie = !!this.cookieStorage.get(this.options.cookieName + this._storageSuffix);


moved into the try catch to ensure that this._storageSuffix is defined

i kind of like that you're using the cookie to signal if the user has accepted tracking

i feel like that's the safest bet to carry over. i haven't fully thought of the case where our cookie expires tho.... 😬

djih

If it's not too much of a hassle can you add a test case for verifying that deferring is skipped if the user already has a cookie

djih · 2019-12-05T23:41:18Z

src/amplitude-client.js

    this.options.apiKey = apiKey;
    this._storageSuffix = '_' + apiKey + this._legacyStorageSuffix;

+    var hasExistingCookie = !!this.cookieStorage.get(this.options.cookieName + this._storageSuffix);


i kind of like that you're using the cookie to signal if the user has accepted tracking

djih · 2019-12-05T23:42:16Z

src/amplitude-client.js

    this._storageSuffix = '_' + apiKey + this._legacyStorageSuffix;

+    var hasExistingCookie = !!this.cookieStorage.get(this.options.cookieName + this._storageSuffix);
+    if (opt_config && opt_config.deferInitialization && !hasExistingCookie) {


note for existing users, they won't get the option to defer tracking because they will already have a cookie, i guess we just need to communicate that to our customers and the people that requested this feature

yes, this will only work on new users without existing cookies and they have to add the config flag

djih · 2019-12-05T23:43:22Z

src/amplitude-client.js

 */
 AmplitudeClient.prototype.setDomain = function setDomain(domain) {
+  if (this._shouldDeferCall()) {
+    return this._q.push(['setDomain', domain]);


is it weird to return the result of array.push? or are you trying to shorthand

{ this._q.push... return }

shorthand. im doing this for the set__ where it's not expecting anything in return

djih · 2019-12-05T23:45:23Z

src/amplitude-client.js

+ */
+AmplitudeClient.prototype._deferInitialization = function _deferInitialization(apiKey, opt_userId, opt_config, opt_callback) {
+  this._initializationDeferred = true;
+  this._q.push(['init', apiKey, opt_userId, opt_config, opt_callback]);


I hope this doesn't cause some weird circular queueing behavior where you defer a function that pushes to this or something, etc 🤔 (seems fine I guess?)

…ice.call(arguments) and add additional tests to verify that existing users (e.g. users with an amplitude cookie) will continue to be logged

sputh · 2019-12-06T21:46:31Z

@djih for sanity's sake, I took your suggestion and added a test to ensure that existing users (e.g. user already has a cookie), will continue to send events even if deferInitialization is set to true

I've also updated the snippet and the minor version.

sputh commented Dec 4, 2019

View reviewed changes

djih self-requested a review December 5, 2019 20:52

djih requested changes Dec 5, 2019

View reviewed changes

djih reviewed Dec 5, 2019

View reviewed changes

sputh commented Dec 5, 2019

View reviewed changes

djih approved these changes Dec 5, 2019

View reviewed changes

sputh added 2 commits December 6, 2019 13:33

When adding function call into this._q, send all the arguments by .sl…

5a0ed4c

…ice.call(arguments) and add additional tests to verify that existing users (e.g. users with an amplitude cookie) will continue to be logged

Bump Minor Version

44207f3

sputh force-pushed the feature/defer-init branch from c1af073 to 44207f3 Compare December 6, 2019 21:44

sputh merged commit 6bc9065 into amplitude:master Dec 6, 2019

sputh deleted the feature/defer-init branch December 6, 2019 21:55

Add feature to deferInitialization #214

Add feature to deferInitialization #214

Conversation

sputh commented Dec 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haoliu-amp commented Dec 5, 2019

Uh oh!

djih left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djih left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djih left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sputh commented Dec 6, 2019

Uh oh!

Uh oh!

sputh commented Dec 4, 2019 •

edited

Loading