Ej/zustand store - Githubissues

digidem / mapeo-desktop

Local-first mapping and monitoring in remote environments

https://mapeo.app

GNU General Public License v3.0

261 stars 33 forks source link

Ej/zustand store #738

Closed lightlii closed 1 year ago

lightlii commented 1 year ago

Contributor checklist:

[x] My contribution is not related to translations. Please submit translation changes via our Mapeo Crowdin project
[x] My commits are in nice logical chunks with good commit messages
[x] I have walked through the QA Manual Testing Script and updated it if necessary.
[x] If my changes depend upon an update to a dependency, I have updated the package-lock.json file using npm install --package-lock
[ ] My changes have been tested with the mobile app.
[ ] My changes are ready to be shipped to users on Windows, Mac, and Linux using the production version of the application built with electron-builder.

Description

Adds zustand state store with persistence using our current conf store. Versioning is achieved with a STORE_VERSION constant applied to the store, not sure if this is the best way, open to feedback.

closes https://github.com/digidem/mapeo-desktop/issues/735

lightlii commented 1 year ago

ok looks like versioning might be better handled in the conf store? looking at the migrations section here

lightlii commented 1 year ago

added another commit with some types on the store. Not sure they're any good though, Zustand's typescript docs are a bit vague unfortunately...

lightlii commented 1 year ago

Ok I'm slightly more confident in my typings now but still need someone to look over them

lightlii commented 1 year ago

after a little back and forth with @gmaclennan on slack we've decided to replace conf with electron-store. It uses conf under the hood so is theoretically compatible with current persisted configs. I'll go ahead and use this PR to make that change I think since it's so small

lightlii commented 1 year ago

lastly, since we now have a means of persisting state through zustand I suggest removing the createPersistedState. It would be less confusing to have only a single way of persisting state in the app. There's only a single use of it in the app so its a very minor change and again I can roll it into this PR if you agree @ErikSin ?

I haven't been able to confirm it's fully compatible with existing persisted configs but the only thing we're saving is the active tab so at worst user won't see their last tab once when opening the next update which I'd argue is an acceptable trade-off

ErikSin commented 1 year ago

That sounds good! Thanks @lightlii