RFC - Storage Layer for Plugins V2 💿 #638

pooyaj · 2022-10-24T21:39:06Z

Second version of the RFC ( first version here )

Problem: If a plugin needs to use browser storage ( cookies, LS, etc. ), it has to implement the storage handling functionality, and Cookie handling in particular requires decent amount of code. Furthermore, plugins aren't aware of AJS storage configuration, so they may use storage regardless of how users configured the AJS.

In order to enable plugins ( our particular use-case is action destinations ) access the storage layer of AJS, we are creating a universalStorage object during initialization and pass it down to plugins as a dependency when registering plugins. UniversalStorage is an interface over a collection of different storage types like cookie, localStorage, and memory that can persist to or read data from those collection of storage objects.

Few things of note:

Universal storage can be configured with a set of targets: cookie localStorage memory
For User storage, we create three instance of universal storage: one for identity, one for legacy identity, and one for traits
For passing storage to plugins, we create a universal storage with all three targets ( unless storage is disabled ) and pass that down to plugins. Plugins can use all the targets, or they can override the targets when calling set/get methods

changeset-bot · 2022-10-24T21:39:09Z

🦋 Changeset detected

Latest commit: b18d0fb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@segment/analytics-next	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

silesky · 2022-10-26T19:18:04Z

packages/browser/src/core/user/index.ts

@@ -183,18 +200,92 @@ export interface CookieOptions {
  sameSite?: string
 }



Typescript nitpick -- Is there a way we can make this fully generic -- like the event emitter?

That way, you can pass in the object and get fully typed keys and values:

export class UniversalStorage<Data extends Record<string, unknown>> { .. public get<K extends keyof Data>( key: K, storeTypes?: StoreType[] ): Data[K] | null { let val = null for (const store of this.getStores(storeTypes)) { val = store.get<Data[K]>(key as string) // store can be made generic too if (val) { return val } } return null }

would lead to something like:

Great suggestion. Added. Could you please if there is any TS smells in my changes or anything can be improved?

I don't see any of the changes yet -- did you push?

Yes, here is the commit: c17e5fc

I see the build error -- you could force people to always explicitly pass data or, in the rare case we don't care (maybe a test?), we might want to add add a default generic parameter like so:

type StorageObject = Record<string, unknown> class UniversalStorage<Data extends StorageObject> = StorageObject { ... }

I know one person that really thinks default generic parameters can be sometimes "harmful", but they can also be quite convenient

I know one person that really thinks default generic parameters can be sometimes "harmful"

What? But they're so handy!

silesky · 2022-10-26T19:21:51Z

packages/browser/src/core/user/index.ts

+
+    return this.set(
+      key,
+      typeof val === 'number' ? val.toString() : val,


What's the reasoning behind coercing to number here rather than deferring to set? This feels a bit confusing if that makes sense =) Can we move it elsewhere?

This is a legacy behavior. I think the reason seems to be AJS classic may set number cookies, but in AJS next we want to convert those to string ( not sure why, maybe because our cookie library doesn't support them 🤔 ) ... I think the problem with this code shows even more with stronger typings that I just added, hence had to add the @ts-ignore. It breaks the assumption that a given cookie key will always have a certain value type :(

Gotcha, I kind of missed that this logic already existed -- all of this context would be great as a comment!

silesky · 2022-10-26T19:22:52Z

packages/browser/src/core/user/index.ts

+    this.stores.push(store)
+  }
+
+  public getAndSync<T>(key: string, storeTypes?: StoreType[]): T | null {


Something feels a bit off about this method -- it breaks command-query separation, but that's maybe not it (returning data in an update isn't that uncommon) -- maybe calling it just resync or resyncAndGet would be a bit better, to show that its primary function should be to sync (and the "get" is just a kind of an extra).

Silly question: If .set "syncs" all of the stores by default, why do they later need to be resynced? Is something happening outside that would cause them to get out of whack? If so, we might probably still want an explicit sync to make that extra clear.

This feels a bit weird here:

analytics-next/packages/browser/src/core/user/index.ts

Line 396 in 3358219

return this.identityStore.getAndSync(this.anonKey)

I lean towards replacing with a resync() method to make it extra explicit that something weird happened and we might need to resync stores. Would that work?

Same, not a fan of this method either. The reason it exists is probably two folds:

If a value exists in one storage but not others ( for whatever reason, i.e., cookies are cleared but localStorage is not ) it syncs the identities across the storages again

There is some formatting mismatch between AJS classic and next cookies, and this makes sure that old AJS cookies are re-written in the new format as soon as we call getAndSync

The reason not refactoring this is to reduce the blast reduce of this PR. This touches fairly critical part of the code, and don't want to break things in one huge PR.

Same as ^.... I kind of missed that this logic already existed -- these kind of WTF / legacy stuff is the kind of thing I absolutely love as comments.

silesky · 2022-10-28T20:16:14Z

packages/browser/src/core/user/index.ts

+  }
+
+  public set<K extends keyof Data>(
+    key: string,


was this meant to be "K" rather than string?

silesky · 2022-10-28T20:16:42Z

packages/browser/src/core/user/index.ts

+  }
+
+  public getAndSync<K extends keyof Data>(
+    key: string,


was this meant to be "K" rather than string?

chrisradek · 2022-10-28T23:12:25Z

packages/browser/src/core/user/index.ts

+      : this.stores
+  }
+
+  public push(store: Store) {


Is push public so that anyone can write their own implementation and add it?

Good catch, removed it as it is not used anywhere. I think if we see a need to add an storage on the fly after creation of a UniversalStorage object we can add it back.

chrisradek · 2022-10-28T23:19:17Z

packages/browser/src/plugins/segmentio/normalize.ts

@@ -109,7 +113,7 @@ function referrerId(
  }

  if (!disablePersistance) {


We probably don't need this check anymore since the universal storage handles this for us now right?

chrisradek

🚢🇮🇹

silesky

🇮🇷 🇨🇦 🚀

zikaari · 2022-11-04T19:07:51Z

Carrying over the comment from previous PR about scoping the storage access - #594 (review)

silesky · 2022-11-05T00:21:13Z

packages/browser/src/core/user/index.ts

-  get = (_key: string): null => null
-  set = (_key: string, _val: unknown): null => null
-  remove = (_key: string): void => {}
+  getType = (): StoreType => 'cookie'
 }

 export class LocalStorage extends Store {


I know you didn't write this code, but maybe we can remove some of the smells from this class?

get<T>(key: string): T | null { const val = localStorage.getItem(key) // this should be wrapped in a try/catch in case storage is not available if (val) { try { return JSON.parse(val) } catch (e) { // this will just return empty object if it's a parse error? This is a bug waiting to happen -- and `JSON.parse(JSON.stringify(val))` is always a smell return JSON.parse(JSON.stringify(val)) } } return null }

would be more idiomatically written as:

get<T>(key: string): T | null { try { const val = localStorage.getItem(key) ?? null // this should be wrapped in a try/catch in case storage is not available return JSON.parse(val); } catch (e) { console.error(e) return null }

We can probably refactor in a subsequent PR. This code has changed since the comment, and don't want to complicate this PR further.

silesky · 2022-11-05T00:22:16Z

packages/browser/src/core/user/index.ts

  static available(): boolean {
+    if (LocalStorage._available !== undefined) {


If we decide wrap localStorage.getItem and .setItem in a try/catch (per common practice), we can get rid of this extra "availability" logic? I feel like it's more typical to just return null on any thrown errors -- such as those from LS being unavailable.

Also would fix: #631

Other thoughts:

I still remember the CDN complexity brought on by that global state -- having the caching part (like CDN) is an optimization that comes with significant trade offs in making code harder for test and harder to reason about, and adds the possibility of bugs having to do with a stale cache / race conditions. So a few reasons why I'd be interested in brainstorming another approach.

_available won't be minified (per many people reminding me!) -- even if you do private static _available

/my 2 cents =)

Yes, good point. Probably the available can be replaced with try/catch. Another one of those changes that I want to avoid in the scope of this PR. Maybe we can improve in a subsequent work. Also as for the minification, I think this is a bigger problem across the code-base. The regular class methods/properties are all remain un-minified unfortunately. I think bigger piece of work would be to refactor this whole thing to not be class based 😆

Refactored to not rely on caching anymore, but keep availability logic to avoid breaking anything.

silesky · 2022-11-05T00:32:27Z

packages/browser/src/core/user/index.ts

@@ -173,6 +190,8 @@ export class LocalStorage extends Store {
  remove(key: string): void {
    return localStorage.removeItem(key)
  }
+
+  getType = (): StoreType => 'localStorage'


Hey, do we really need this? Typical way of narrowing that is by checking instanceOf -- in general, since it's a class and not a plain object, I don't generally ever seen a reason for labels like this on classes unless it's a like field on a class that's meant to be serialized (like a custom error) .

Revising this comment, after delay. The UniversalStorage will be passed into the action-destinations, and I want the action-destinations to have a nice way to choose their storage targets: i.e., storage.get('test', ['cookies']) ... Not sure possible cleanly here 🤔

Refactored to use getters, at list slightly cleaner/nicer

pooyaj requested review from chrisradek, silesky and a team October 24, 2022 21:42

silesky reviewed Oct 26, 2022

View reviewed changes

silesky reviewed Oct 28, 2022

View reviewed changes

pooyaj force-pushed the pj/universal_storage branch from 1c7c64f to 11a9258 Compare October 28, 2022 23:09

chrisradek reviewed Oct 28, 2022

View reviewed changes

chrisradek approved these changes Oct 28, 2022

View reviewed changes

silesky approved these changes Nov 2, 2022

View reviewed changes

silesky reviewed Nov 5, 2022

View reviewed changes

silesky force-pushed the master branch 2 times, most recently from da031b5 to 275d5a3 Compare November 27, 2022 23:54

pooyaj force-pushed the pj/universal_storage branch from 03daae9 to f1bbdd4 Compare December 7, 2022 00:35

pooyaj added 9 commits December 7, 2022 14:02

universal storage functionality

753d807

minor fixes

6921583

refactor

3ea0045

perf improvement

d14edcf

better typing for UniversalStorage

38e7330

type improvements + tests

630a47e

RIP bundle size

eac73d4

fix tests

e89261e

tests after rebase

6001080

pooyaj added 7 commits December 7, 2022 14:04

cleanup normalize

6ab9ce9

remove the public push interface

656876c

adding back the TS fix that got lost in rebase + sizelimit

54b4ef6

change to getter

bad3c11

bug fix

e84e6d6

fix linting as a result of tsconfig changes

2549028

remove caching logic for availability, refactored the universal storage

9549176

pooyaj force-pushed the pj/universal_storage branch from 1e71aa5 to 9549176 Compare December 7, 2022 22:05

pooyaj added 4 commits December 7, 2022 16:07

lint fix

414baf2

moar tests

8249457

scope the ts-config changes to browser package

fc675b8

changeset

b18d0fb

pooyaj merged commit e16017d into master Dec 8, 2022

pooyaj deleted the pj/universal_storage branch December 8, 2022 00:37

github-actions bot mentioned this pull request Dec 8, 2022

Version Packages #717

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC - Storage Layer for Plugins V2 💿 #638

RFC - Storage Layer for Plugins V2 💿 #638

pooyaj commented Oct 24, 2022 •

edited

Loading

changeset-bot bot commented Oct 24, 2022 •

edited

Loading

silesky Oct 26, 2022 •

edited

Loading

pooyaj Oct 28, 2022

silesky Oct 28, 2022

pooyaj Oct 28, 2022

silesky Oct 28, 2022 •

edited

Loading

chrisradek Oct 28, 2022

silesky Oct 26, 2022 •

edited

Loading

pooyaj Oct 28, 2022

silesky Oct 28, 2022

silesky Oct 26, 2022 •

edited

Loading

pooyaj Oct 28, 2022

silesky Oct 28, 2022 •

edited

Loading

silesky Oct 28, 2022 •

edited

Loading

silesky Oct 28, 2022

chrisradek Oct 28, 2022

pooyaj Oct 28, 2022

chrisradek Oct 28, 2022

chrisradek left a comment

silesky left a comment

zikaari commented Nov 4, 2022

silesky Nov 5, 2022 •

edited

Loading

pooyaj Dec 7, 2022

silesky Nov 5, 2022 •

edited

Loading

pooyaj Dec 7, 2022

pooyaj Dec 7, 2022

silesky Nov 5, 2022 •

edited

Loading

pooyaj Dec 7, 2022

pooyaj Dec 7, 2022

silesky Dec 7, 2022

		@@ -183,18 +200,92 @@ export interface CookieOptions {
		sameSite?: string
		}

		@@ -109,7 +113,7 @@ function referrerId(
		}

		if (!disablePersistance) {

		static available(): boolean {
		if (LocalStorage._available !== undefined) {

RFC - Storage Layer for Plugins V2 💿 #638

RFC - Storage Layer for Plugins V2 💿 #638

Conversation

pooyaj commented Oct 24, 2022 • edited Loading

changeset-bot bot commented Oct 24, 2022 • edited Loading

🦋 Changeset detected

silesky Oct 26, 2022 • 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

silesky Oct 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silesky Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silesky Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silesky Oct 28, 2022 • edited Loading

Choose a reason for hiding this comment

silesky Oct 28, 2022 • 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

chrisradek left a comment

Choose a reason for hiding this comment

silesky left a comment

Choose a reason for hiding this comment

zikaari commented Nov 4, 2022

silesky Nov 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silesky Nov 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silesky Nov 5, 2022 • 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

pooyaj commented Oct 24, 2022 •

edited

Loading

changeset-bot bot commented Oct 24, 2022 •

edited

Loading

silesky Oct 26, 2022 •

edited

Loading

silesky Oct 28, 2022 •

edited

Loading

silesky Oct 26, 2022 •

edited

Loading

silesky Oct 26, 2022 •

edited

Loading

silesky Oct 28, 2022 •

edited

Loading

silesky Oct 28, 2022 •

edited

Loading

silesky Nov 5, 2022 •

edited

Loading

silesky Nov 5, 2022 •

edited

Loading

silesky Nov 5, 2022 •

edited

Loading