Skip to content
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

fix: assign domain after cookie storage options are given #528

Merged
merged 2 commits into from
May 11, 2022
Merged

fix: assign domain after cookie storage options are given #528

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8638570
fix: Assign domain after cookie storage options are added
dnshi May 9, 2022
c27699a
fix: address comment
dnshi May 9, 2022
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
2 changes: 1 addition & 1 deletion src/amplitude-client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ AmplitudeClient.prototype.init = function init(apiKey, opt_userId, opt_config, o
secure: this.options.secureCookie,
sameSite: this.options.sameSiteCookie,
});
this.options.domain = this.cookieStorage.options().domain;
Copy link
Contributor

Choose a reason for hiding this comment

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

im curious how this impacts sdk usages that did not use defer init. would they have existing cookie without the domain suffix? if we move it up to this line, then would it look for cookie with the domain prefix, which it may not find?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is really interesting that we actually have two cookie names: with domain and without domain.
image
image

And it does not defer init
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When initializing the metadata storage without domain, all cookies saved by the metadata storage does not include the domain suffix. After domain is assigned to this.options.domain, SDK looks for cookie but cannot find it, and then a new cookies with domain suffix is created. I think this is the flow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm not sure about that, but isn't metadata storage only instantiated once for the most part? then does this mean even if this.options.domain changes, it would still use the initial storage key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isn't metadata storage only instantiated once for the most part?

If deferInitialization is not set, then it is only one metadata storage instance. But for defer init, a new metadata storage instance is initialized with the new domain value.

then does this mean even if this.options.domain changes, it would still use the initial storage key?

If this.options.domain changes, the storage key is still the initial one.

Here we have two options:

  1. Move the this.options.domain = this.cookieStorage.options().domain; before initialize metadata storage, so the cookies include domain suffix.
  2. Move the this.options.domain = this.cookieStorage.options().domain; after the defer init check, so the metadata storage is initialized again, the storage key is still the same.

I pick the first one option, but I am thinking if you have any other concern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From setDomain method, the this.options.domain is updated immediately.

Amplitude-JavaScript/src/amplitude-client.js

Lines 862 to 880 in 52abaf0

AmplitudeClient.prototype.setDomain = function setDomain(domain) {
if (this._shouldDeferCall()) {
return this._q.push(['setDomain'].concat(Array.prototype.slice.call(arguments, 0)));
}
if (!utils.validateInput(domain, 'domain', 'string')) {
return;
}
try {
this.cookieStorage.options({
expirationDays: this.options.cookieExpiration,
secure: this.options.secureCookie,
domain: domain,
sameSite: this.options.sameSiteCookie,
});
this.options.domain = this.cookieStorage.options().domain;
_loadCookieData(this);
_saveCookieData(this);

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah my concern is what I mentioned in my first comment. Like if they don't defer init, which is default behavior, and client devices have an existing cookie key with no suffix. If we go with option 1, then it will not find the existing cookies right? If it does not find existing cookies, then it will generate new session ids and device ids and potentially disassociate events that would have been for the same amplitude ID, I'm thinking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I think it is a valid concern. I will move to option 2.


this._metadataStorage = new MetadataStorage({
storageKey: this._cookieName,
Expand All @@ -141,7 +142,6 @@ AmplitudeClient.prototype.init = function init(apiKey, opt_userId, opt_config, o
const hasNewCookie = !!this._metadataStorage.load();
this._useOldCookie = !hasNewCookie && hasOldCookie && !this.options.cookieForceUpgrade;
const hasCookie = hasNewCookie || hasOldCookie;
this.options.domain = this.cookieStorage.options().domain;

if (this.options.deferInitialization && !hasCookie) {
this._deferInitialization(apiKey, opt_userId, opt_config, opt_callback);
Expand Down