Local Storage Configuration Helper #5691
Conversation
…to local-storage-config-helper
| } | ||
| }); | ||
|
|
||
| export function createLocalStorageConfigurationProxy<C extends Record<string, unknown>>( |
There was a problem hiding this comment.
having some comments about classes and functions would be nice :)
| @@ -0,0 +1,78 @@ | |||
| import { Lazy } from "./lazy"; | |||
|
|
|||
| const isLocalStorageAvailable = new Lazy(()=>{ | |||
There was a problem hiding this comment.
we already have something like that in ODSP driver. Can we consolidate?
There was a problem hiding this comment.
i'd like to see the ODSP driver just use this. i updated a couple examples, but there are more
| } | ||
| } | ||
|
|
||
| public get(target: T, p: string): any { |
There was a problem hiding this comment.
what about set()? If I overwrite whole property/object, we should reflect that in this.configCache, right?
There was a problem hiding this comment.
My thought was config will be immutable, as it gets complex it config change underneath
| .get(p); | ||
|
|
||
| case "undefined": | ||
| // we don't know the type of the prop, see if sub-props exist, in which case assume |
There was a problem hiding this comment.
This is a bit ugly, in a sense that there are obvious paths when things will go wrong.
It assumes only narrow usage patterns, which may be not obvious for the caller.
I do not know if I have better suggestion, but maybe we say that all these configs are read-only? I.e. consumer (runtime) can only read, that would substantially reduce wrong-doing by callers
There was a problem hiding this comment.
it def needs better docs, and error handling before checkin. the big question i have is, can you safely use proxy. if so, then it's worth to do all those things
| }); | ||
|
|
||
| export function createLocalStorageConfigurationProxy<C extends Record<string, unknown>>( | ||
| namspace: string, target: C): Readonly<C> { |
There was a problem hiding this comment.
you can see here I return the object as read-only
|
|
||
| export class LocalStorageConfigurationProxyHandler<T extends Record<string, unknown>> implements ProxyHandler<T> { | ||
| private readonly configCache = new Map<string, unknown>(); | ||
| constructor(private readonly namespace: string) { |
There was a problem hiding this comment.
Think about how something like nconf can be wrap on the node side as well to allow argv/env/json config files to provide options.
There was a problem hiding this comment.
i was thinking about that exact thing, and i'm not sure. it's an open question to me if we should setup the proxy, like I'm doing here, or ask partners to do it, basically they should wrap it before they give it to us. I see benefits and draw backs to both, so not sure which direction to go
| // the target property is an object, so construct a new proxy for it | ||
| return this.configCache.set( | ||
| p, | ||
| new Proxy(targetValue ?? {}, new LocalStorageConfigurationProxyHandler(localStorageKey))) |
There was a problem hiding this comment.
Is the '??' for null? (...if so, probably want to set the cache to an unproxied null.)
| // not a object, and not in local storage, so return the in-memory value | ||
| return this.configCache.set(p, targetValue).get(p); | ||
| } | ||
| } |
There was a problem hiding this comment.
Just FYI - IIRC, the additional handlers (ownKeys, getOwnPropertyDescriptors, etc.) are required to avoid breaking features that use reflection, such as the spread operator, JSON stringify, etc.
|
closing for #5921 |
Proxy all our options bags to local storage, so we can easily override things at runtime
related to #5396