Refactor LocalStorage and associated hooks
#11625
Conversation
somebody1234
added
CI: No changelog needed
somebody1234
requested review from
PabloBuchu,
indiv0 and
MrFlashAccount
as code owners
|
@MrFlashAccount for CR |
|
and i guess @indiv0 (or @MrFlashAccount) for QA |
🧪 Storybook is successfully deployed!📊 Dashboard:
|
| const { localStorage } = useLocalStorage() | ||
|
|
||
| useEffect(() => { | ||
| if ('error' in schema.safeParse(localStorage.get(key))) { |
There was a problem hiding this comment.
I think this won't notify subscribers about removing the item from localstorage.
Also I'm not a fan of doing this stuff on the hook level, I though the defineLocalStorageKey is just a wrapper that removes the need to write boilerplate stuff, and the whole logic is still in the LocalStorage component
There was a problem hiding this comment.
I think this won't notify subscribers
notifying logic should still be in the LocalStorage class right now
not a fan of doing this stuff on the hook level
true. but now the schema stuff no longer exists in the LocalStorage class, i guess not strongly opinionated on it though. is there a reason schema stuff should be in LocalStorage?
(previously it was there mostly because all the registry stuff was more or less global - so it wasn't an option at all for the schema to be not in the core class)
There was a problem hiding this comment.
I thought we're stil into keeping it global? And this is supposed to be a solution for import order
There was a problem hiding this comment.
yeah, but global doesn't mean all the functionality needs to be in one file, right? is there a reason the schema stuff should be in the LocalStorage class? like, i guess imo the best solution to kinda enforce this is to avoid exposing an alternative way with the same API that uses just the class
There was a problem hiding this comment.
Yeah, if we want developers to fall into the pit of success by forcing them to use defineLocalStorageItem, we need to put these api into the same folder and expose only the function, but not the LocalStoage class. But I still think that the LocalStorage should keep the schema, as it used to be, because we could have different wrappers around the class in the future and coping this logic might be a bad idea.
There was a problem hiding this comment.
btw not sure to have it in a separate module (so the class needs to be exported) but also hide it from auto import
| return [value, setValue] | ||
| return [value, setValue] | ||
| } | ||
| return { get, set, delete: deleteKey, useGet, useSet, useDelete, useState: useLocalStorageState } |
There was a problem hiding this comment.
can we unify the stuff here, like:
// use array to make it easier to rename
return [
item: {
get,set,clear
},
useItem: useLocalStorageState(): [value, setValue, clearValue]
]Hook might be less performant but more convinient to use.
For perf reasons we might expose setter as third item:
...
useSetItem(): [setValue, clearValue]There was a problem hiding this comment.
use array to make it easier to rename
not sure about this one, especially since you don't always need both non-reactive and reactive parts.
i think unifying the values as you suggested should be enough
There was a problem hiding this comment.
Same as for useState. We can flip items in the array btw. Also we can declare names for the array items aswell
There was a problem hiding this comment.
Same as for useState
i know useState does it but IMO that's because a getter/setter pair is a very common pattern for reactive state - and because you almost always want to rename the accessors anyway, and because you usually want all (all two) members of the tuple
There was a problem hiding this comment.
And we dont want to rename the result of 'defineLocalStorageItem'?
There was a problem hiding this comment.
you do have a point! still not 100% sure as we don't always want all the members and it's a lot less obvious what each returned value is (it's not a popular pattern unlike get/set), but if you think it's better than the current option then we can absolutely do that
| } = defineLocalStorageKey('localRootDirectories', { | ||
| schema: (z) => z.string().array().readonly(), | ||
| }) | ||
| export const { use: useLocalRootDirectories, useState: useLocalRootDirectoriesState } = |
There was a problem hiding this comment.
Why is it still use? Is it a hook?
There was a problem hiding this comment.
yup. it uses localStorage from the context, not sure if it's necessary tbh but i think removing the LocalStorageProvider is out of scope for this PR?
There was a problem hiding this comment.
Nowadays LocalStorage is a Singleton class, we can avoid using it from the context to make it possible to manipulate the storage state outside of the components, I think
…k when registering
…t to skip re-render
somebody1234
requested review from
Frizi,
farmaazon,
vitvakatu,
kazcw and
AdRiley
as code owners
Pull Request Description
localStorageto use functionality returned by a single function, rather than registering keys plugin-style when modules are loaded.Important Notes
None
Testing Instructions
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
The documentation has been updated, if necessary.Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,or the Snowflake database integration, a run of the Extra Tests has been scheduled.
If applicable, it is suggested to paste a link to a successful run of the Extra Tests.