Module unload and persistent module atoms #717
Conversation
rubenthoms
changed the title
Co-authored-by: Ruben Thoms <[email protected]>
…dpoints back-end (#684) Co-authored-by: Hans Kallekleiv <[email protected]> Co-authored-by: Ruben Thoms <[email protected]>
There was a problem hiding this comment.
In general, a good implementation 👍 Some remarks and comments.
As already discussed, this is very likely not the persistence mechanism we’re going to use in the long run as it lacks a layer of abstraction. The unload mechanism for module instances, however, might become relevant in other scenarios.
|
|
||
| /** | ||
| * Creates an writeable atom that persists it's value in localstorage, but keeps it unique per module instance. **Note: must only be used within a module instances own store** | ||
| * @param storageKey Prefix for the key. Should be unique among a module's atoms. Actual storage key will be: `<prefix>:<moduleInstanceId>` | ||
| * @param initialValue Initial atom value | ||
| * @returns A get/set atom | ||
| */ | ||
| export function atomWithModuleInstanceStorage<T>(storageKey: string, initialValue: T) { | ||
| // simple atom, needed to trigger new lookups | ||
| const instanceValueAtom = atom<T | null>(null); | ||
|
|
||
| return atom<T, [T], void>( | ||
| (get) => { | ||
| const instanceId = getIntanceId(get); | ||
| const fullStorageKey = makeInstanceStorageKey(instanceId, storageKey); | ||
| const instanceValue = get(instanceValueAtom); | ||
| const storedValue = localStorage.getItem(fullStorageKey); | ||
|
|
||
| if (instanceValue !== null) return instanceValue; | ||
| else if (storedValue !== null) return JSON.parse(storedValue); | ||
| else return initialValue; | ||
| }, | ||
| (get, set, newVal: T) => { | ||
| const instanceId = getIntanceId(get); | ||
| const fullStorageKey = makeInstanceStorageKey(instanceId, storageKey); | ||
|
|
||
| localStorage.setItem(fullStorageKey, JSON.stringify(newVal)); | ||
| set(instanceValueAtom, newVal); | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| export function clearModuleInstanceStorage(instanceId: string, key: string) { | ||
| const fullKey = makeInstanceStorageKey(instanceId, key); | ||
| localStorage.removeItem(fullKey); | ||
| } | ||
|
|
||
| function makeInstanceStorageKey(instanceId: string, key: string): string { | ||
| // ! Be mindful about changing this; existing stored values will dissapear! | ||
| return `${key}:${instanceId}`; | ||
| } | ||
|
|
||
| function getIntanceId(get: Getter): string { | ||
| const id = get(CurrentModuleInstanceIdAtom); | ||
|
|
||
| if (id === null) { | ||
| throw new Error("Module instance not set. Make sure this atom is only used within a module storage"); | ||
| } | ||
|
|
||
| return id; | ||
| } |
There was a problem hiding this comment.
First of all, this looks like a clean and nice implementation.
However, I am a bit concerned about this getting mixed up/confused with persistence in the longer run. Especially, since we have not decided yet to what extent we would like to persist modules. One scenario is to leave a dashboard and be able to get back to it exactly the way it was, no matter what machine/browser you were on. This would include overall states (both data and GUI) of the framework and also local module states (also both data and GUI). The other scenario is to just persist data-relevant states and not GUI states (such as which drawer/group is open etc.).
In the first scenario, this implementation would conflict with the other persistence implementation. In the latter, this could work as an addition to it (GUI states only per browser+user). However, the question is also if atoms should have their own persistence mechanism or not as they're usually not used by GUI components. Persistence of framework GUI elements is already managed by the GuiMessageBroker class. Other GUI elements could have a persistable state (usePersistedState). So where would atoms be in this setup?
Since all of this is not discussed yet, I would rather not want this implementation to leak out as I'm afraid others might start using it for their implementations making it harder to adjust to the persistence system we decide on in the longer run. I would prefer if the Well-log viewer module used a temporary internal way of storing its data.
There was a problem hiding this comment.
Thats a fair concern. Im fine with closing down the PR and then we can come back to it later. Would it be okay to use this persistence solution privately inside the well-log viewer module still?
If we close the PR for now; since you noted that the onInstanceUnload would be nice, I could open a new PR that only introduces that logic by itself if wanted
There was a problem hiding this comment.
One could argue if «experimental» is the right term as this implies that this might become non-experimental at some point with a quite high certainty. Here, however, it might very well become obsolete due to the other persistence implementation. Another term could be «temporary» to highlight its temporary character.
There was a problem hiding this comment.
In my opinion, experimental is correct for something like this; I see "experimental" as implying something is equally likely to get added as it is to be removed, and therefore should be used with caution. "Temporary" is telling me that the thing 100% will be removed, which is not what my intention was, originally.
Even if it's unlikely we'll add it - as voiced in your earlier comment - we might decide to keep it, so I don't think "temporary" is correct.
atomWithModuleInstanceStorage. This one can be used to persist module-atoms in localStorage, keeping them unique for each instance