Input local storage tweaks

fdoflorenzano commented 2 years ago

Hey! Just posting this as a response to the already merged #140

The main tweak I wanted to add is adding key sorting to the key hash generation. Since the hash is computed from a string representation of a copy of the inputs export object, the order of the keys become important to generate the hash. This made the updated setup (introduced in #140) create a new storage even when changing the order of keys in the input, or even keys inside the values for inputs, which should (I think) preserve the storage for that definition.

One thing that worried me about the PR was that it maybe resets values for the inputs too frequently, even if it's not needed in certain scenarios. Like maybe an number input value I tweaked into a very specific value already in the UI, but if I tweak the max value in the definition it will reset the current value. But of course, what if that max value is smaller than the current value? There may be a lot of edge cases that are weird to control, but at some point we could introduce smarter resets. The current setup is simpler in that sense.

But another thing I worry it introduces is adding much more new entries in local storage. I started worrying back when we introduced seed history storage, cause this too already seems like a lot saved there. So here I also add a little clearLocalStorage function that could be called from the UI in certain scenarios (like debugging or something crashes and we add to the error log that they can clear it). Tho I don't know yet how or where to add that in the UI. Any ideas welcome :)

netlify[bot] commented 2 years ago

Deploy Preview for dsi-logo-maker ready!

Name	Link
Latest commit	7156fecc086141ba85e3bb57e69255e78a9c5296
Latest deploy log	https://app.netlify.com/sites/dsi-logo-maker/deploys/632a14625714fc0009dc9fae
Deploy Preview	https://deploy-preview-150--dsi-logo-maker.netlify.app/
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

runemadsen commented 2 years ago

Thanks for the comments @fdoflorenzano! I was so excited about this but should probably have waited with the merge until you were back. I propose that we jump on a call to discuss the best way forward for this!

lucasdinonolte commented 2 years ago

Thanks for the improvements! I think your observations and worries about my initial naive implementation of this are spot on! I think discussing the details of this in quick chat makes most sense. I'm just leaving some initial ideas here:

I'm not sure how much of an issue too many local storage entries are performance wise, but maybe a simple heuristic to clean up could be keep track of the current local storage key. If the key gets changed (because the inputs change) we could just delete the current key, before setting current key to the new key
- I also like your idea of adding a UI option as this might be the more explicit solution
I think localStorage is stored on hard drive, so maybe there's a minor performance gain in debouncing localStorage writes?
I would like to brainstorm if there could be a simple heuristic of when to reset the cache key and when to keep the old one without things becoming too complex. Maybe we could also avoid resetting all values to their default by merging the contents of the previous local storage cache with the new local storage. This could maybe be done fairly simple (if nothing about the input was changed -> copy the old value, if something about the input was changed -> reset the value to default)

fdoflorenzano commented 2 years ago

Closed in favor of #155, moved conversation there

runemadsen commented 2 years ago

A quick weird idea: what if each input had its own localStorage entry? Then we would not reset the other inputs when a single input changes.

On Thu, Sep 15, 2022 at 9:27 PM Lucas Nolte @.***> wrote:

Thanks for the improvements! I think your observations and worries about my initial naive implementation of this are spot on! I think discussing the details of this in quick chat makes most sense. I'm just leaving some initial ideas here:

I'm not sure how much of an issue too many local storage entries are performance wise, but maybe a simple heuristic to clean up could be keep track of the current local storage key. If the key gets changed (because the inputs change) we could just delete the current key, before setting current key to the new key

I also like your idea of adding a UI option as this might be the more explicit solution

I think localStorage is stored on hard drive, so maybe there's a minor performance gain in debouncing localStorage writes?

I would like to brainstorm if there could be a simple heuristic of when to reset the cache key and when to keep the old one without things becoming too complex. Maybe we could also avoid resetting all values to their default by merging the contents of the previous local storage cache with the new local storage. This could maybe be done fairly simple (if nothing about the input was changed -> copy the old value, if something about the input was changed -> reset the value to default)

— Reply to this email directly, view it on GitHub https://github.com/designsystemsinternational/mechanic/pull/150#issuecomment-1248521283, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABO4FLUH4ZZJ3ITPHBQIXTV6N2D3ANCNFSM6AAAAAAQMRYNMA . You are receiving this because your review was requested.Message ID: @.***>

fdoflorenzano commented 2 years ago

Interesting! That's a good idea. Currently the whole inputs object is saved together as a single entry in localStorage, and reset all together, so to do this probably some re-writing would be needed for how the localStorage entries are handled, or having individual entries for each single input, for every single design function, which seems a bit much, but I guess isn't that bad.

designsystemsinternational / mechanic