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

Add support for chrome.storage backed configuration store. #63

Merged
merged 7 commits into from
May 17, 2024

Conversation

leoromanovsky
Copy link
Member

@leoromanovsky leoromanovsky commented May 15, 2024


labels: mergeable

Fixes: #issue

Motivation and Context

Users in the chrome extension environment do not have access to localStorage but need to persist configuration between sessions.

Description

  • adds support for chrome.storage backed AsyncStore
  • factory method detects if chrome is available and instantiated the configuration store
  • 👉 attempted a unit test for the configuration factory - with all the mocks it might have ended up a mess. suggestions welcome.

How has this been tested?

@leoromanovsky leoromanovsky force-pushed the lr/ff-2083/chrome-async branch from 0316318 to 136ce0b Compare May 15, 2024 04:51
@leoromanovsky leoromanovsky marked this pull request as ready for review May 15, 2024 04:57
Base automatically changed from lr/ff-1980/no-init2 to main May 16, 2024 22:35
@@ -83,7 +87,7 @@ export interface IClientConfig {
export { IAssignmentLogger, IAssignmentEvent, IEppoClient } from '@eppo/js-client-sdk-common';

// Instantiate the configuration store with memory-only implementation.
const configurationStore = configurationStorageFactory(undefined, true);
const configurationStore = configurationStorageFactory({ forceMemoryOnly: true });
Copy link
Member Author

Choose a reason for hiding this comment

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

I like this API more - to me it reads more clearly what is being performed.

Comment on lines +183 to +184
chromeStorage: hasChromeStorage() && chrome.storage.local,
windowLocalStorage: hasWindowLocalStorage() && window.localStorage,
Copy link
Member Author

Choose a reason for hiding this comment

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

IMO not great to have to pass these in with the hasChromeStorage protection but we don't know if the APIs are available otherwise.

@leoromanovsky leoromanovsky merged commit c961b14 into main May 17, 2024
2 checks passed
@leoromanovsky leoromanovsky deleted the lr/ff-2083/chrome-async branch May 17, 2024 22:02
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.

2 participants