Implementing beforeRead and beforeWrite #250

bertmad3400 · 2024-04-26T17:57:03Z

Implemented the beforeRead and beforeWrite methods described in #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:

The input shape to the serializer stringify function and the output shape from the serializer stringify function is now no longer the shape of content of the store, but rather any, as the functions beforeRead and beforeWrite are intended to be used to transform the data into the shape of the store, but it is worth keeping in mind that the serializer functions are now typed with any instead of a specific shape
This does however bring some interesting possibilities like storing a version number only in the localStorage and the use these proxy functions (beforeRead and beforeWrite) to not only validate this version number, but to strip it if it isn't of use for the application

It should also be noted, that as discussed in #244 (comment), I have removed the behavior where the store contents would be set to null if the storage event contents are null. As mentioned by #244 (comment) 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

Previously it would set the store value to null, which would break with the type safety

joshnuss

Left one comment about null handling.

We should probably update the README (can just add examples of the signature in the options)

Maybe in the future we can have a section in the README about migration using beforeRead and beforeWrite. I can write something after merging.

joshnuss · 2024-04-26T19:53:36Z

test/localStorageStore.test.ts

+      const store = persisted("beforeRead-init-test", 0, { beforeRead: (v) => v * 2 })
+      expect(get(store)).toEqual(4)
+    })
+    it("allows modfiying value before reading upon event", () => {


should be "modifying" 😸

Omg, my spelling is actually terrible

test/localStorageStore.test.ts

bertmad3400 · 2024-04-27T08:58:09Z

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

bertmad3400 · 2024-04-27T09:12:26Z

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 · 2024-05-05T02:48:47Z

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 · 2024-05-05T02:49:33Z

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?

Uses a symbol which can be returned by either function to cancel the operation

bertmad3400 · 2024-05-16T12:29:29Z

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 · 2024-05-16T12:29:58Z

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 · 2024-05-20T20:02:25Z

index.ts

@@ -57,7 +57,10 @@ export function persisted<T>(key: string, initialValue: T, options?: Options<T>)
  const storage = browser ? getStorage(storageType) : null

  function updateStorage(key: string, value: T) {
-    const newVal = beforeWrite(value)
+    const cancel = Symbol("cancel")


Why the need for a symbol?
Can't beforeWrite just return a value?

As described before, the symbol here is used to cancel the write operation. Imagine a case where the beforeWrite function would want to cancel the writing to localStorage (this could maybe be because of invalid data, because of cases like #217, or maybe something else). As we don't know of the shape of the store data, beforeWrite should be able to return any kind of value (primitives (numbers, strings, etc.), objects, arrays and even null/undefined) and have it written to localStorage. As such, if we want to implement cancel by return value, we have to create a value that we know is unique, and that we know that beforeWrite couldn't possibly return, unless it's explicitly our cancel value, which is why symbols are used (as this is their entire purpose). The only other way of implementing a cancel operation would be to passe in a cancel function that the when called would cancel the write operation, but not only is this more code in the library, but I also believe it to be more confusing for the user, as the return value suddenly have no effect then.

I'm still unclear on why beforeWrite() should be allowed to cancel and how often this is needed. (Just trying to keep implementation and docs simple)

Cancelation means "keep the last value", but we could do that with a return statement. for example

beforeWrite(val) { // cancel write by writing last value return JSON.parse(localStorage.getItem(key)) }

But still unclear on why anyone would want to cancel a write, and let the store's state get out of sync with local storage.

Se there's a couple of sides to this:

First of, while the code you presented here technically allows for keeping the same value, I don't think it's very nice for the user to have to write that. The reason they're using this library is to avoid accessing localStorage in the first place.

Second of, what you have written here isn't actually equivalent to cancelling the operation, due to the fact that just returning the same value as currently present will still write that value to localstorage. This has a couple of implications:

It means a case like the one described in Delete from storage when setting back to default value #217, where there shouldn't be written anything to localStorage before the user has accepted it, becomes near impossible. I do realize that a potential solution could be to just write null to localStorage as I would imagine this would be fine for the legislation, but honestly, I don't know and I do find it to be a messy solution.

It means that there's no way to - on a case by case basis - prevent the store from triggering the re-rendering on other tabs as even writting the same value to localStorage will send the localStorage event. I personally work with a particular case where this is needed, as a lot of the DOM rendering is dependent on the store value (which can be really heavy for the users device to rerender), and as such we only want to sync the store across tabs under specific conditions - something which this would enable, and which is really difficult to do under the current implementation (I do realize the existence of the sync option, but that doesn't work on a case-by-case basis). I would imagine that there would be other cases like this

Ultimately, it would be up to you whether we want to keep this functionality or not (I can just remove it if not desired), but I would personally recommend for it, both due to the previously mentioned cases (where I don't see a simple solution without it), and because I believe - from a developer perspective - that it's a simple and intuitive solution. Just returning the existingVal to beforeRead would mean that the input signatures for beforeRead and beforeWrite would be different, and will - in my mind - definitely at some point lead to developers asking us "It's great I can prevent updates through beforeRead, but how do I do the same for beforeWrite?". I think the solution of keeping the input signature, and the mechanism for cancelling (by returning the cancel param) the same for both functions is the simplest for developers to understand and get used to.

To clarify, I was questioning why cancel is needed for beforeRead & before write.

I think it's overkill for this library right now.

As with all decisions, I'm open to changing it if more people ask for it.

It's important when building things to target the 80% - what most people want. If we try to target 100% this project will get very complicated, and it will impact the users and the maintenance.

I appreciate your work on this, but I think we should not worry about edge cases untill we hear many people need this.

Got it. While I'm still not sure I agree, I definitely see your point, and will just go ahead and remove this functionality.

…e-Write and -Read

joshnuss · 2024-05-23T14:04:47Z

README.md

@@ -51,6 +51,8 @@ export const preferences = persisted('local-storage-key', 'default-value', {
  syncTabs: true, // choose whether to sync localStorage across tabs, default is true
  onWriteError: (error) => {/* handle or rethrow */}, // Defaults to console.error with the error object
  onParseError: (raw, error) => {/* handle or rethrow */}, // Defaults to console.error with the error object
+  beforeRead: (value) => {/* change value after serialization but before setting store to return value. Return cancel to cancel the operation*/},


Small thing: Can we remove the messaging about cancel for beforeWrite and beforeRead

Whoopsi, got it fixed now

joshnuss · 2024-05-23T14:05:32Z

index.ts

@@ -21,71 +21,85 @@ export interface Serializer<T> {

 export type StorageType = 'local' | 'session'

-export interface Options<T> {
-  serializer?: Serializer<T>
+export interface Options<StoreType, SerializerType> {


Should the SerializerType default to StoreType? or will it be a breaking change

I think the trouble here is the same as in your other question regarding whether to include more types, and as such much of my answer is the same. Technically though, no, SerializerType and StoreType are two different types, as beforeRead may be used to alter the shape coming from deserialize to fit the store shape

joshnuss · 2024-05-23T14:07:10Z

index.ts

 }
-export function persisted<T>(key: string, initialValue: T, options?: Options<T>): Writable<T> {
+export function persisted<StoreType, SerializerType>(key: string, initialValue: StoreType, options?: Options<StoreType, SerializerType>): Writable<StoreType> {


Are more types absolutely required? This will impact everyone using TypeScript even if they don't use beforeRead/beforeWrite

Yeah, that was one of my pet peeves as well with this change. I would love to have you look over the types quickly, and see if you have a better solution. Currently, this is the most accurate solution, but thinking about it, it might be better to just type the output of Parse and input to Stringify as unknown

Uh yeah, just experimented with it and typing it as unknown definitely isn't the right move (as this requires immense data-validation efforts on the app-dev side, something better handled by just handling errors when they arrive). I think we might have to keep the split types, although I'm open to potential suggestions

What about setting a default type for SerializerType?

persisted<T, SerializerType = T>(...)

Then it's not a breaking change. And people that don't use beforeRead/Write aren't impacted.

Also, I don't use TS, so I'd rather get the opinion from people that do (p.s. I feel like its a little weird that I am maintaining a TS lib, kinda wish it was just JS, and let TS people define a .d.ts)

Damn, you don't do TS? I guess, if you mainly do libraries it makes sense, but as an app developer, TS over JS is the single biggest productivity improvement I have made in my 10 years of programming. I could never, ever, never go back to untyped code-bases.

Regarding this, I agree your solution is awesome! I wasn't actually aware that this was possible, but just implemented it, so I think we are ready to merge. Once again, thank you for your awesome work on this library!

joshnuss · 2024-05-23T14:08:58Z

index.ts

-
-    if (json) {
+  function maybeLoadInitial(): StoreType {
+    function serialize(json: any) {


Is there a reason for extracting this function?
I think it's only called once, inside the current scope

I actually didn't introduce this function (merely changed its type) and as such I deemed it to be outside the scope of this PR (though yes, I agree, it shouldn't be a separate, scope-wide function). It was introduced back in d77aeaa, as a fix for #231

joshnuss · 2024-05-31T15:22:30Z

Thanks @bertmad3400!!

You rock sir! Thanks for sticking with this.

github-actions · 2024-05-31T15:22:57Z

🎉 This PR is included in version 0.10.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

bertmad3400 added 3 commits April 26, 2024 17:49

Make persisted ignore events when value is null

f7c1927

Previously it would set the store value to null, which would break with the type safety

Add beforeWrite and beforeRead methods

32553f3

Implement tests for beforeRead and beforeWrite

77ac10d

bertmad3400 mentioned this pull request Apr 26, 2024

Lifecycle hook to modify / upgrade data. #244

Closed

joshnuss reviewed Apr 26, 2024

View reviewed changes

bertmad3400 added 2 commits April 27, 2024 08:42

Fix spelling mistakes

15a0a88

Add syntax example for beforeRead and beforeWrite to readme

49ab7f5

bertmad3400 added 4 commits May 16, 2024 12:24

Add cancel operation to beforeRead and beforeWrite

ff269f0

Uses a symbol which can be returned by either function to cancel the operation

Add seperate store and serializer types

9e0c092

Fix single type-error

81a918c

Update README

b3b985f

joshnuss reviewed May 20, 2024

View reviewed changes

Revert implementation, documentation and testing for cancelling befor…

f89f201

…e-Write and -Read

joshnuss reviewed May 23, 2024

View reviewed changes

bertmad3400 added 2 commits May 23, 2024 14:37

Remove mention of cancel param from readme

007544c

Set StoreType as the default shape for SerializerType

cb0a317

joshnuss merged commit 1a83f84 into joshnuss:master May 31, 2024
4 checks passed

github-actions bot added the released label May 31, 2024

wqyeo mentioned this pull request Jul 10, 2024

[Snyk] Upgrade svelte-persisted-store from 0.9.2 to 0.11.0 Muimi-Chat/admin-client#1

Open

wqyeo mentioned this pull request Jul 10, 2024

[Snyk] Upgrade svelte-persisted-store from 0.9.2 to 0.11.0 Muimi-Chat/chat-client#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing beforeRead and beforeWrite #250

Implementing beforeRead and beforeWrite #250

bertmad3400 commented Apr 26, 2024

joshnuss left a comment

joshnuss Apr 26, 2024

bertmad3400 Apr 27, 2024

bertmad3400 commented Apr 27, 2024

bertmad3400 commented Apr 27, 2024

joshnuss commented May 5, 2024

joshnuss commented May 5, 2024

bertmad3400 commented May 16, 2024

bertmad3400 commented May 16, 2024

joshnuss May 20, 2024

bertmad3400 May 21, 2024

joshnuss May 21, 2024

bertmad3400 May 22, 2024

joshnuss May 22, 2024

bertmad3400 May 23, 2024

joshnuss May 23, 2024

bertmad3400 May 23, 2024

joshnuss May 23, 2024

bertmad3400 May 23, 2024

joshnuss May 23, 2024

bertmad3400 May 23, 2024

bertmad3400 May 23, 2024

joshnuss May 23, 2024

bertmad3400 May 26, 2024

joshnuss May 23, 2024

bertmad3400 May 23, 2024

joshnuss commented May 31, 2024

github-actions bot commented May 31, 2024

Implementing beforeRead and beforeWrite #250

Implementing beforeRead and beforeWrite #250

Conversation

bertmad3400 commented Apr 26, 2024

joshnuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertmad3400 commented Apr 27, 2024

bertmad3400 commented Apr 27, 2024

joshnuss commented May 5, 2024

joshnuss commented May 5, 2024

bertmad3400 commented May 16, 2024

bertmad3400 commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshnuss commented May 31, 2024

github-actions bot commented May 31, 2024