ai_user cookie not present after reenabling the cookie #1585 #1632
Conversation
MSNev
commented
Aug 11, 2021
MSNev
commented
- Add optimization to avoid always updating the cookie value
- Add optimization to avoid always updating the cookie value
MSNev
requested review from
ramthi,
xiao-lix,
hectorhdzg,
kryalama and
Karlie-777
| @@ -75,6 +83,8 @@ export class User implements IUserContext { | |||
| const params = cookie.split(User.cookieSeparator); | |||
| if (params.length > 0) { | |||
| _self.id = params[0]; | |||
| // we already have a cookie | |||
| _self.isUserCookieSet = !!_self.id; | |||
There was a problem hiding this comment.
If we successfully read a value from the cookie then we assume that the cookie was set.
| @@ -98,7 +108,7 @@ export class User implements IUserContext { | |||
| // set it to 365 days from now | |||
| // 365 * 24 * 60 * 60 = 31536000 | |||
| const oneYear = 31536000; | |||
| _cookieManager.set(_storageNamePrefix(), cookie, oneYear); | |||
| _self.isUserCookieSet = _cookieManager.set(_storageNamePrefix(), cookie, oneYear); | |||
There was a problem hiding this comment.
Use new response that tells us if the cookie was really set.
| let user_cookie = _generateNewCookie(user_id); | ||
| _setUserCookie(user_cookie.join(User.cookieSeparator)); | ||
| // Optimizations to avoid setting and processing the cookie when not needed | ||
| if (_self.id !== userId || !_self.isUserCookieSet) { |
There was a problem hiding this comment.
Add an extra performance based check so we don't try and set the cookie if
- The value was already set
- The new userId is the same as the previous one.
| } | ||
|
|
||
| _processTelemetryInternal(event, itemCtx); | ||
|
|
||
| if (theContext.user && theContext.user.isNewUser) { | ||
| theContext.user.isNewUser = false; | ||
| if (userCtx && userCtx.isNewUser) { |
There was a problem hiding this comment.
Simple minification improvement which also has a miner perf improvement.
| /** | ||
| * A flag indicating whether the user cookie has been set | ||
| */ | ||
| isUserCookieSet?: boolean; |
There was a problem hiding this comment.
Adding as optional so we don't break anyone who is using this interface
| */ | ||
| set(name: string, value: string, maxAgeSec?: number, domain?: string, path?: string): void; | ||
| set(name: string, value: string, maxAgeSec?: number, domain?: string, path?: string): boolean; |
There was a problem hiding this comment.
While this is a change to the signature of the function, it should be backward compatible as previously it was void.
There was a problem hiding this comment.
This is also used by internally as well, so this should be compatible with that as well.
