ajy-UID2-1667-Local-storage-for-v3-js-sdk #21

alex-yau-ttd · 2023-09-11T04:55:08Z

Add useCookie option for init.
Migrate legacy cookie to new cookie
Use local storage by default (when useCookie is not provided)
Use the new uid2StorageManager to manage cookie vs local storage
Prettier changes
Typo fixes

Tests

Create some new tests for the useCookie option in options.test.ts
Run lots of existing tests over three scenarios using testCookieAndLocalStorage.
Add some new mocks, and adjusting other mocks and their associated assertions

- Migrate legacy cookie to new cookie - Add useCookie option - use localStorage by default

lionell-pack-ttd · 2023-09-11T05:11:17Z

src/uid2CookieManager.ts

@@ -71,12 +71,20 @@ export class UID2CookieManager {
    }
  }

+  private migrateLegacyCookie(identity: LegacyUid2SDKCookie, now: number): Uid2Identity {
+    const newCookie = enrichIdentity(identity, now);
+    this.setCookie(newCookie);


If the publisher is close to the cookie size limit, there's a chance this will fail - I think it'd be better to stick with the existing approach of enriching it and returning it, rather than storing the enriched cookie.

The enriched cookie is just the new cookie right? How do we plan to migrate from legacy cookie to the new (enriched) cookie if not by storing the enriched cookie? Can we just have them use local storage if they are close to the cookie size limit?

There's a good chance we'd be going from the legacy cookie straight into local storage, so if we just return it enriched here and store it in whatever is trying to upgrade it, then the common case won't ever increase the cookie size.

Actually - my comment's not quite right.

A legacy cookie will eventually get replaced by a new one - the next time the identity is updated (probably when it gets refreshed). So if we just enrich it when we read it (and don't write it back), it'll eventually be stored in the new format - hopefully in local storage instead. But that then breaks the load, because the migrated value will look newer than anything in local storage. Hmmm.

Thinking through a few options, I think maybe your initial approach is the right move here - just upgrade it and store it back immediately.

lionell-pack-ttd · 2023-09-11T05:13:34Z

src/uid2LocalStorageManager.ts

+export class UID2LocalStorageManager {
+  public setValue(identity: Uid2Identity) {
+    const value = JSON.stringify(identity);
+    localStorage.setItem("identity", value);


identity probably has a high chance of conflicting with other things - I think it'd be a good idea to choose a localstorage ID that's prefixed with UID2.

true. would UID2 itself also run the risk of conflicts? Or is it just too vague? Perhaps UID2-identity?

Alternatively, cookies use __uid_2. Would it make sense to use the same name or would that be more confusing?

We sometimes find publishers store things for UID2 as well - I think making it clear that it's stored by the SDK would be helpful in a lot of cases. Maybe something like UID2-sdk-identity.

lionell-pack-ttd · 2023-09-11T05:17:20Z

src/uid2Sdk.ts

+        this._cookieManager.setCookie(validity.identity);
+      }
+      else {
+        this._cookieManager.removeCookie();


I like the idea of removing the cookie here, but we'll need to be very sure there aren't any cases where the cookie might be removed if we haven't loaded it properly yet - we don't want to remove a valid cookie if we aren't definitely going to store it.

to be extra defensive, I could change getValue to a public method and do:

this._localStorageManager.setValue(validity.identity); if (this._localStorageManager.getValue()) this._cookieManager.removeCookie();

Would that address your concern?

Yeah, I think that's reasonable.

lionell-pack-ttd · 2023-09-11T05:18:47Z

src/uid2Sdk.ts

+      identity = this._opts.identity;
+    } else {
+      const localStorageIdentity = this._localStorageManager.loadIdentityFromLocalStorage();
+      identity = localStorageIdentity !== null ? localStorageIdentity : this._cookieManager.loadIdentityFromCookie();


You could probably use ?? to simplify this a bit.

I've changed how this works

alex-yau-ttd · 2023-09-14T03:40:05Z

src/uid2Sdk.ts

+    } else {
+      const localStorageIdentity = this._localStorageManager.loadIdentityFromLocalStorage();
+      const cookieIdentity = this._cookieManager.loadIdentityFromCookie();
+      const shouldUseCookie = cookieIdentity && (!localStorageIdentity || cookieIdentity.identity_expires > localStorageIdentity.identity_expires);


Hopefully this handles the case we talked about @lionell-pack-ttd, where the cookie is newer than the value in localStorage, so we should use the cookie instead.

lionell-pack-ttd · 2023-09-25T01:19:34Z

src/Uid2Options.ts

@@ -6,7 +6,7 @@ import { InitCallbackOptions } from "./Uid2InitCallbacks";
 export type Uid2Options = BaseUid2Options &
  InitCallbackOptions &
  UID2CookieOptions &
-  Uid2ApiClientOptions;
+  Uid2ApiClientOptions & { useCookie?: boolean; };


I think you could add useCookie to the base options type - I think that's just there for options that are just for uid2sdk.ts and not provided by one of the other classes/files.

lionell-pack-ttd · 2023-09-25T01:31:39Z

src/uid2LocalStorageManager.ts

+export class UID2LocalStorageManager {
+  public setValue(identity: Uid2Identity) {
+    const value = JSON.stringify(identity);
+    localStorage.setItem("UID2-identity", value);


Avoid magic strings. Make sure there's a single source of truth for UID2-identity - in some cases using a type to enforce the string being correct everywhere is an option, but I don't think we can do that here so a constant is probably the move.

As I mentioned in another comment, we often end up with a few different UID2 values from different sources (i.e. the publisher storing their own things), so I think something like UID2-sdk-identity would make it easier to trouble-shoot integrations.

defined and exporting the const from this file now - I'm using it in the mocks as well (although I don't have to)

- Use const for localStorageKeyName - Move useCookie to base options - Check if value is in localStorage before removing cookie

- There are still 8 tests failing in 4 suites

Without this, only the first pass of the test with succeed, and the following ones would file since the window.cypto would not change

Wraps uid2CookieManager and uid2LocalStorageManager, so the sdk only needs the uid2StorageManager, offloading logic off the sdk class.

Also adjust the mock to return undefined instead of null for localStorage for parity with cookie mock

jingyi-gao-ttd · 2023-09-28T01:48:35Z

src/mocks.ts

@@ -163,6 +177,20 @@ export function getUid2Cookie() {
  }
 }

+export function removeUid2LocalStorage() {
+  localStorage.removeItem(localStorageKeyName);


this looks exact the same as removeValue in UID2LocalStorageManager? Why we couldn't use that instead?

jingyi-gao-ttd · 2023-09-28T01:48:47Z

src/mocks.ts

+
+export function setUid2LocalStorage(identity: any) {
+  const value = JSON.stringify(identity);
+  localStorage.setItem(localStorageKeyName, value);


same as above

jingyi-gao-ttd · 2023-09-28T01:49:26Z

src/mocks.ts

+
+export function getUid2LocalStorage() {
+  const value = localStorage.getItem(localStorageKeyName);
+  return value !== null ? JSON.parse(value) : undefined;


Could we use loadIdentityFromLocalStorage?

I see what you mean, my current implementation seems like a bit of copy-paste work and would still mean tests would pass if the actual functionality broke (BAD).

Are you suggesting that this mocks file should have a

const uid2LocalStorageManager = new UID2LocalStorageManager();

and then be

export function getUid2LocalStorage() { return uid2LocalStorageManager.loadIdentityFromLocalStorage(); }

I wonder if we need them in the mocks.ts?
Could we do const uid2LocalStorageManager = new UID2LocalStorageManager(); in test file and just call uid2LocalStorageManager functions?

yeah I think that makes sense. In fact I think I can actually use

const uid2StorageManager = new UID2StorageManager({});

then just do

uid2StorageManager.removeValues();

rather than having

removeUid2Cookie(); removeUid2LocalStorage();

I've made some changes, though I still have getUid2LocalStorage() as a mock so that it will return undefined instead of null. Alternatively, I could change loadIdentityFromLocalStorage() to return Uid2Identity | undefined instead of Uid2Identity | null and then remove the need for getUid2LocalStorage()

I initially based it off loadIdentityFromCookie(): Uid2Identity | null but I think maybe we can use undefined instead of null. What do you think?

tbh I am getting a bit confused myself. - perhaps @lionell-pack-ttd could chime in next week. I see in getUid2Cookie() we can return undefined (so this is what the tests expect), whereas in loadIdentityFromCookie() we can return null. Would it make sense to just use null or undefined everywhere, across SDK, tests, cookies & local storage?

What are we achieving by switching from the original approach to loadIdentityFromLocalStorage? Test infrastructure shouldn't rely on the things it's testing. If the goal is to test whether a value is set in local storage (or clear the value from local storage), then using the UID2LocalStorageManager to do that is a problem.

For example, with this approach, if I change UID2LocalStorageManager to this:

The tests all still pass, even though UID2LocalStorageManager doesn't use localStorage at all and would be horribly broken in a real context (it would never persist any data and always start as null on each page load).

thanks for the feedback Lionell, I've reverted the change so that I'm using getUid2LocalStorage again. One thing I wanted to check with you again @lionell-pack-ttd - are you okay with the implementation of getUid2LocalStorage?

Specifically, the : undefined is necessary because the mock method getUid2Cookie returns undefined, and we have tests that run for both local storage and cookie that assert on toBeUndefined().

Other alternatives includes:

Changing getUid2Cookie to return null if !docCookie, then change getUid2LocalStorage to return null instead of undefined

Changing the assertions to be toBeFalsy() instead of toBeUndefined(). The issue with this is that it also includes false, 0 and '' which I don't particularly like - as far as I can tell that is the only way to cover toBeNull() || toBeUndefined()

I'd lean towards the first alternative unless you feel strongly about it.

The first approach seems like a good one to me - but I don't really feel strongly here, whichever you think is best.

I've gone with this approach:

Changing getUid2Cookie to return null if !docCookie, then change getUid2LocalStorage to return null instead of undefined

jingyi-gao-ttd · 2023-09-28T01:56:34Z

src/uid2Sdk.ts

@@ -17,6 +17,8 @@ import {
  isClientSideIdentityOptionsOrThrow,
 } from "./uid2ClientSideIdentityOptions";
 import { bytesToBase64 } from "./uid2Base64";
+import { UID2LocalStorageManager } from "./uid2LocalStorageManager";


nit: this import is never used

src/uid2StorageManager.ts

jingyi-gao-ttd · 2023-09-28T02:03:33Z

src/uid2StorageManager.ts

@@ -0,0 +1,47 @@
+import { UID2 } from "./uid2Sdk";
+import { isValidIdentity, Uid2Identity } from "./Uid2Identity";


nit: isValidIdentity and UID2 is not used

src/uid2StorageManager.ts

alex-yau-ttd · 2023-09-28T04:27:20Z

src/uid2StorageManager.ts

+    this._localStorageManager = new UID2LocalStorageManager();
+  }
+
+  public loadIdentityFromPreferredStorageWithFallback(): Uid2Identity | null {


This probably deserves some tests

- Clean up imports - Rename usage of loadIdentityFromPreferredStorageWithFallback - Return early in `setValue` method

jingyi-gao-ttd · 2023-09-28T06:06:09Z

src/integrationTests/async.spec.ts

+
+testCookieAndLocalStorage(() => {
+  describe("when getAdvertisingTokenAsync is called before init", () => {
+    describe("when initialising with a valid identity", () => {


Do we cover test cases for when initialising without identity? (which calls the loadIdentityWithFallback?)

Do we have any test that testing if the refreshed token has been stored to the correct storage?

jingyi-gao-ttd · 2023-09-28T06:08:43Z

src/integrationTests/async.spec.ts

+        });
+        const p = uid2.getAdvertisingTokenAsync();
+        uid2.init({ identity: originalIdentity, useCookie: useCookie });
+        xhrMock.responseText = btoa(


nit: maybe we could wrap xhrMock.responseText and xhrMock.onreadystatechange into a mockResponse(response) function?

- Use localStorageManager methods in mocks or in tests directly - Remove ununsed imports

lionell-pack-ttd · 2023-10-04T05:53:16Z

src/uid2StorageManager.ts

+import { Uid2Options } from "./Uid2Options";
+
+export class UID2StorageManager {
+  private _cookieManager: UID2CookieManager | undefined;


I dropped you a DM about this - I think the coverage is showing as low on this file because of these | undefined, and these values will always be set once the constructor has run.

Drop | undefined from managers inside UID2StorageManager and associted null coalescing. Was not necessary and gave confusing code coverage insights

This reverts commit f9d20ae.

alex-yau-ttd added 3 commits September 11, 2023 14:54

Local storage option for v3 js sdk

69338de

- Migrate legacy cookie to new cookie - Add useCookie option - use localStorage by default

Prettier changes

7b25acc

Migrate from cookie to localStorage

0e045ae

lionell-pack-ttd reviewed Sep 11, 2023

View reviewed changes

alex-yau-ttd added 2 commits September 13, 2023 10:50

Use cookie identity if later expipry

3428da6

change localStorage key

4754e1f

alex-yau-ttd commented Sep 14, 2023

View reviewed changes

lionell-pack-ttd reviewed Sep 25, 2023

View reviewed changes

alex-yau-ttd added 8 commits September 25, 2023 13:18

Addressing feedback

1fbd943

- Use const for localStorageKeyName - Move useCookie to base options - Check if value is in localStorage before removing cookie

WIP Tests

410b8b1

- There are still 8 tests failing in 4 suites

Add writeable: true to mockObject.defineProperty

15c4cc8

Without this, only the first pass of the test with succeed, and the following ones would file since the window.cypto would not change

Clean up basic.test.ts

764d158

Remove cookie & localStorage in top beforeEach

bd64b39

Use new uid2StorageManager

65d6f04

Wraps uid2CookieManager and uid2LocalStorageManager, so the sdk only needs the uid2StorageManager, offloading logic off the sdk class.

Check for cookie/localStorage instead of cookie

f5542c2

Also adjust the mock to return undefined instead of null for localStorage for parity with cookie mock

Fix typo expection -> exception

2b5e1ad

alex-yau-ttd marked this pull request as ready for review September 27, 2023 07:05

alex-yau-ttd added 2 commits September 27, 2023 17:44

use mock getUid2 for tests

7416a7b

Use setUid2 instead of setting cookie & identity

c9c312c

jingyi-gao-ttd reviewed Sep 28, 2023

View reviewed changes

alex-yau-ttd commented Sep 28, 2023

View reviewed changes

alex-yau-ttd added 2 commits September 28, 2023 14:27

Rename method that loads identity

bf79519

Addressing feedback

b78fe26

- Clean up imports - Rename usage of loadIdentityFromPreferredStorageWithFallback - Return early in `setValue` method

jingyi-gao-ttd reviewed Sep 28, 2023

View reviewed changes

Addressing comments

f9d20ae

- Use localStorageManager methods in mocks or in tests directly - Remove ununsed imports

lionell-pack-ttd reviewed Oct 4, 2023

View reviewed changes

alex-yau-ttd added 3 commits October 4, 2023 16:57

Address feedback

5311892

Drop | undefined from managers inside UID2StorageManager and associted null coalescing. Was not necessary and gave confusing code coverage insights

Revert "Addressing comments"

fe714e3

This reverts commit f9d20ae.

Storage mocks return null, update assertions

736c841

lionell-pack-ttd approved these changes Oct 9, 2023

View reviewed changes

alex-yau-ttd merged commit be8ebf4 into main Oct 9, 2023

alex-yau-ttd deleted the ajy-UID2-1667-Local-storage-for-v3-js-sdk branch October 9, 2023 04:17

		@@ -0,0 +1,47 @@
		import { UID2 } from "./uid2Sdk";
		import { isValidIdentity, Uid2Identity } from "./Uid2Identity";

ajy-UID2-1667-Local-storage-for-v3-js-sdk #21

ajy-UID2-1667-Local-storage-for-v3-js-sdk #21

Conversation

alex-yau-ttd commented Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-yau-ttd Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-yau-ttd commented Sep 11, 2023 •

edited

Loading

alex-yau-ttd Sep 28, 2023 •

edited

Loading