Passing Storage to Plugins #594
Conversation
|
| private localStorage: Store | ||
| private mem: Store | ||
|
|
||
| options: UserOptions = {} |
There was a problem hiding this comment.
Hah, good catch, has no use ( remains of copy 🍝 )
| autoBind(this) | ||
| } | ||
|
|
||
| chainGet<T>(key: string): T | null { |
There was a problem hiding this comment.
Pedantic: The word "chain" and "try" seem like implementation details, any reason to not continue to use the conventions of the web storage API (if not outright implementing it?). I feel like universal storage could implement the same interface as the other types of storage (sorry if I'm repeating myself).
| @@ -398,3 +398,53 @@ export class Group extends User { | |||
| return undefined | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Is the idea that this utility class can also be shared with the User class -- either injected or through inheritance?
There was a problem hiding this comment.
Yes! either of those two. So either User inherits from UniversalStorage or we just inject the _storage instance to User class. Going to refactor those after we settle of overall approach.
|
|
||
| export class UniversalStorage { | ||
| private cookies: Store | ||
| private localStorage: Store |
There was a problem hiding this comment.
Looking at this file, my instinct is that it would make sense to create an actual shared interface and implement it --- it might make future testing stubs easier and ensure some unity between all these similar storage classes and makes moving to injection easier (where appropriate)
| @@ -127,6 +135,8 @@ export class Analytics | |||
| : options?.group, | |||
| cookieOptions | |||
| ).load() | |||
|
|
|||
| this._storage = new UniversalStorage(disablePersistance, cookieOptions) | |||
There was a problem hiding this comment.
Looking at the code, It'd be nice to be able to inject an instance of this class into the User's constructor!
| ): Promise<void> { | ||
| await Promise.resolve(plugin.load(ctx, instance)) | ||
| await Promise.resolve(plugin.load(ctx, instance, pluginConfig)) |
There was a problem hiding this comment.
The plugin will use this as pluginConfig.dependencies.storage. It kinda looks bit abrasive to me, why not just pass it in as a flattened argument. It is highly unlikely that there'll be anything more we'll need to make available to plugins.
There was a problem hiding this comment.
We should probably make a "scoped" instance of storage accessible to each plugin instead of the "shared" storage. Although highly unlikely, but there might be a case where 2 or more plugins are using the same key and writing conflicting values, or worse using the clear operation.
Here's what I mean:
const createScopedStorage = (storage: UniversalStorage, scope: string) => ({
get: (key: string) => storage.get(`${scope}::${key}`),
set: (key: string, value: any) => storage.set(`${scope}::${key}`)
})
|
|
||
| options: UserOptions = {} | ||
|
|
||
| constructor(isDisabled: boolean, cookieOptions?: CookieOptions) { |
There was a problem hiding this comment.
Can we allow selectively choosing what is enabled and disabled? Maybe that means replacing isDisabled with something like
interface StorageOptions {
cookies: boolean,
localStorage: boolean,
inMemory: boolean
}
or
interface Storage {
cookie: Store
localStorage: Store
inMemory: Store
}
(Option 2 gets rid of cookieOptions as well 😄)
Just thinking of all the different permutations we support/want to support (just cookies, just localstorage, all of the above, nothing)
There was a problem hiding this comment.
Thinking of this more - instead of option 2, could even pass in an array of stores - we don't really need to know if they are cookies or localstorage or whatever.
There was a problem hiding this comment.
@chrisradek doesn’t the user only generally use one type of storage here at a time? I see this class as being a pre-configured utility class that works for typical browser like environments — basically the UniverstalStorage class is a sensible default final class that users can use if they want to. IMO if they need customization, they should be directed to pass a custom class to the plug-in options that implements a generic storage interface.
There was a problem hiding this comment.
@pooyaj I would maybe consider removing the isDisabled as a configuration option — If feels slightly wrong to have a class responsible for stubbing out its own methods — that this is the responsibility of common interfaces, and it perhaps feels more idiomatic to move the isDisabled/NullStorage logic up a level.
silesky
force-pushed
the
master
branch
2 times, most recently
from
da031b5
to
275d5a3
Compare
RFC - Storage Layer for Plugins 💾
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.
Few things of note:
PluginConfigobject to pass the dependencies to the plugin. Better options?Userclass andUniversalStorage, so there is some opportunity for refactoring.[] I've included a changeset (psst. run
yarn changeset. Read about changesets here).