joshnuss / svelte-persisted-store

A Svelte store that persists to localStorage
MIT License
994 stars 41 forks source link

Implementing beforeRead and beforeWrite #250

Closed bertmad3400 closed 3 months ago

bertmad3400 commented 4 months ago

Implemented the beforeRead and beforeWrite methods described in https://github.com/joshnuss/svelte-persisted-store/issues/244. When considering this PR it's important to have the following in mind:

The type of the data stored in localStorage may no longer be the same or even have the same shape as the data stored in the store with the corresponding key. This is absolutely intentional, but it does bring some "quirks" when it comes to stuff like typing, as the data shapes may be different:

It should also be noted, that as discussed in https://github.com/joshnuss/svelte-persisted-store/issues/244#issuecomment-2066114385, I have removed the behavior where the store contents would be set to null if the storage event contents are null. As mentioned by https://github.com/joshnuss/svelte-persisted-store/issues/244#issuecomment-2071318361 this is a type check specifically to ensure that the value of the event isn't null (and it's important), but the correct response here should be to avoid a null value

bertmad3400 commented 4 months ago

Thank you for your quick response @joshnuss (and your amazing work on this library)! Hope the changes fixes your concerns

bertmad3400 commented 4 months ago

So I have been doing some thinking, and it annoys me that we don't have any pre-read validation capability. What I mean is, what if a value is written to local storage, the event fires, the persisted store serializes the value, and it then reaches beforeRead, but we in beforeRead decides to discard the value and want to keep the current value? I could see plenty of scenarios where this would be the case (imagine migration where we don't truly want to migrate the data, but just use the initial value every time the data in local storage is out of date). Honestly, I think the same capability would be useful for beforeWrite (to cancel the write to localStorage).

Now, the question becomes, how do we do this? I considered passing in the currentValue in beforeRead to enable the developer to just return that, but the same approach doesn't really work for beforeWrite. We could also maybe passe in a function to beforeRead and beforeWrite which when called would cancel the operation. What do you think would be the cleanest?

joshnuss commented 4 months ago

I think passing the value makes sense, for example:

beforeRead(val, existing) {
  // abort, return existing
  return existing
}

Also makes it easy to skip:

beforeRead(val) {
  // no need to take extra parameter
}
joshnuss commented 4 months ago

Otherwise this PR looks good! Thanks @bertmad3400!!

Should I merge? or do you want to add the new parameter to beforeRead/beforeWrite in this PR?

bertmad3400 commented 3 months ago

Yeah, so I wasn't 100% satisfied with the current solution, especially as it would leave the beforeWrite without a cancel operation, and would also make this a rather asymetric solution. As such, I had a thought and some discussion, and ended up implementing a cancel parameter that if returned (instead of a new value) cancels this operation. Under this hood, this uses symbols to differentiate between what could be a new value for the store, and the cancel param

bertmad3400 commented 3 months ago

I would love for someone to quickly look over the typing (as it has become rather complex), but except for that, I think we are good to merge

joshnuss commented 3 months ago

Thanks @bertmad3400!!

You rock sir! Thanks for sticking with this.

github-actions[bot] commented 3 months ago

:tada: This PR is included in version 0.10.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: