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.

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

feat: storage options, change priority or use custom #908

Merged
merged 6 commits into from
Aug 23, 2023
Merged

Conversation

oscb
Copy link
Contributor

@oscb oscb commented Jul 20, 2023

Adds a storage option to analytics client.

This can be used to specify a particular order for storage systems. e.g.

storage: [StoreType.Cookie, StoreType.LocalStorage, StoreType.Memory]

or

storage: ['cookie', 'localStorage', 'memory']

This is useful for web apps that might use multiple domains/subdomains to use cookies over local storage.

  • This PR also moves class implementations from the User package to Storage as User had become too bloated.
  • The UniversalStorage option for overriding particular StoreType is removed as the behaviour was getting complex to maintain if the order is also switchable. It is recommended to create a UniversalStorage object with the particular systems instead.
  • Refactored the UniversalStorage arguments to prevent passing around cookieOptions as an specific thing all over the place. New API should be more extensible if other storages require any additional settings.
  • Refactored the Store class to reuse the same interface of UniversalStorage as they were pretty much the same.
  • Adds additional tests.

WIP: Custom Storage support

[✔︎] I've included a changeset (psst. run yarn changeset. Read about changesets here).

@changeset-bot
Copy link

changeset-bot bot commented Jul 20, 2023

🦋 Changeset detected

Latest commit: 8332060

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
Copy link
Contributor

silesky commented Jul 20, 2023

Awesome! there's a new open issue I think this would partly solve (complete web worker support is TBD, but gets us closer) #907

Copy link
Contributor

@danieljackins danieljackins left a comment

Choose a reason for hiding this comment

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

Looks awesome! Just some questions/nits but nothing blocking

packages/browser/src/core/analytics/index.ts Outdated Show resolved Hide resolved
packages/browser/src/core/storage/settings.ts Outdated Show resolved Hide resolved
packages/browser/src/core/storage/settings.ts Outdated Show resolved Hide resolved
packages/browser/src/core/user/index.ts Outdated Show resolved Hide resolved
packages/browser/src/core/storage/types.ts Outdated Show resolved Hide resolved
@kyranjamie
Copy link

Great to see this being worked on. Note that chrome.storage.local has an asynchronous API, so ideally this will work with Promises too

getItem(): Promise<any> | any;
setItem(): Promise<any> | any;
removeItem(): Promise<any> | any;

Copy link
Contributor

@silesky silesky left a comment

Choose a reason for hiding this comment

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

Thanks for this! There's a lot to like about this PR, such as the refactoring. Blocking for some more discussion/review of this API, since there is no SDD.

Can we base Storage on the idiomatic Web Storage API (and/or with AsyncStorage support per @kyranjamie comment), rather than using the same interface as UniversalStorage? The UniversalStorage interface is both larger than I think it needs to be and confusing and I would hesitate to commit to this as our main public API for storage behavior.

Other concerns I have as a user are similarly around UniversalStorage and the ways it differs from what I'd expect from a typical storage interface.

  • storage.getAndSync vs storage.get -- the former is an implementation detail. I'd be confused how to implement these and why if I'm just adding some basic storage implementatio, and what I'm expecting to be syncing with if there's only one store. Consumers can power up their storage implementation whichever way they want. if they want to combine cookies and indexDB into one class, they can go ahead. Maybe we don't need to complicate our interface.
  • Why do I need to implement 'type' if storage only accepts a single store instance?
  • can we make .available not required?

@oscb
Copy link
Contributor Author

oscb commented Jul 26, 2023

  • storage.getAndSync vs storage.get -- the former is an implementation detail. I'd be confused how to implement these and why if I'm just adding some basic storage implementatio, and what I'm expecting to be syncing with if there's only one store. Consumers can power up their storage implementation whichever way they want. if they want to combine cookies and indexDB into one class, they can go ahead. Maybe we don't need to complicate our interface.

Agree on this. getAndSync doesn't make much sense anymore if we're killing the weird support of overriding the Storages to use in each operation. I'm fine with leaving this up to the Storage implementation

Why do I need to implement 'type' if storage only accepts a single store instance?

I think this was due to an old approach I took (?) doesn't seem like I need this anymore. Fine with removing it.

can we make .available not required?

I actually think we should keep this one. That way you can do fallbacks of Storage systems abstracting a little bit more of how you know if they are ready.

I'm ok with switching from the UniversalStorage interface, just wanted to stick to it since there's at least one destination using it currently and didn't want to break stuff

@oscb oscb requested a review from silesky August 8, 2023 17:29
@oscb oscb merged commit 1b95946 into master Aug 23, 2023
3 checks passed
@oscb oscb deleted the oscb/storage branch August 23, 2023 21:01
@github-actions github-actions bot mentioned this pull request Aug 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants