-
Notifications
You must be signed in to change notification settings - Fork 0
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
Provide a customer persistence store (FF-2085) #65
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
export function configurationStorageFactory(): IConfigurationStore<Flag> { | ||
if (hasWindowLocalStorage()) { | ||
export function configurationStorageFactory( | ||
persistenceStore?: IAsyncStore<Flag>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming to persistentStore
, which I think is what this is usually called, unless I'm misunderstanding what this is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I think it makes sense as an argument in init rather than a usePersistentStore(store)
method that could modify the client's storage after it has been initialized. If there's a scenario where swapping stores is useful, it doesn't seem like we're blocked from providing that method in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I think it makes sense as an argument in init rather than a usePersistentStore(store) method that could modify the client's storage after it has been initialized
That was my thinking too - can't imagine wanting to swap this out in the middle of a session.
labels: mergeable
Fixes: #issue
Motivation and Context
Allow customers to provide a customer persistence store implementation of
IAsyncStore
. They can pair this with (#64) to create an "off-line" mode where the client CDN will not make any remote requests. In this case they would provide aIAsyncStore
that reads from a local file (or memory) for a configuration that was provided by their server.Another use case is for AWS Lambda functions, not in scope for this PR as that is nodejs, which are very short-lived servers. Their persistence store can be backed by DynamoDB to avoid many CDN requests.
Description
persistenceStore
- if provided aHybridConfigurationStore
will be created with itquestions for reviewers
In order to provide the persistence store, I needed to instantiate the Client singleton within the
init
method. Does this seem like an acceptable compromise? The alternative is adding ausePersistenceStore()
method onto js-commons.How has this been tested?