Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementing beforeRead and beforeWrite #250

Merged
merged 12 commits into from
May 31, 2024
Merged

Implementing beforeRead and beforeWrite #250

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f7c1927
Make persisted ignore events when value is null
bertmad3400 Apr 26, 2024
32553f3
Add beforeWrite and beforeRead methods
bertmad3400 Apr 26, 2024
77ac10d
Implement tests for beforeRead and beforeWrite
bertmad3400 Apr 26, 2024
15a0a88
Fix spelling mistakes
bertmad3400 Apr 27, 2024
49ab7f5
Add syntax example for beforeRead and beforeWrite to readme
bertmad3400 Apr 27, 2024
ff269f0
Add cancel operation to beforeRead and beforeWrite
bertmad3400 May 16, 2024
9e0c092
Add seperate store and serializer types
bertmad3400 May 16, 2024
81a918c
Fix single type-error
bertmad3400 May 16, 2024
b3b985f
Update README
bertmad3400 May 16, 2024
f89f201
Revert implementation, documentation and testing for cancelling befor…
bertmad3400 May 23, 2024
007544c
Remove mention of cancel param from readme
bertmad3400 May 23, 2024
cb0a317
Set StoreType as the default shape for SerializerType
bertmad3400 May 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ export interface Options<T> {
onError?: (e: unknown) => void
onWriteError?: (e: unknown) => void
onParseError?: (newValue: string | null, e: unknown) => void
beforeRead?: (val: any) => T
beforeWrite?: (val: T) => any
beforeRead?: <S extends symbol>(val: any, cancel: S) => T | S
beforeWrite?: <S extends symbol>(val: T, cancel: S) => any | S
}

function getStorage(type: StorageType) {
Expand Down Expand Up @@ -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")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Se there's a couple of sides to this:

  1. 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.
  2. 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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const newVal = beforeWrite(value, cancel)
if (newVal === cancel) return

try {
storage?.setItem(key, serializer.stringify(newVal))
} catch (e) {
Expand All @@ -74,8 +77,15 @@ export function persisted<T>(key: string, initialValue: T, options?: Options<T>)
}
}
const json = storage?.getItem(key)
if (json == null) return initialValue

const serialized = serialize(json)
if (serialized == null) return initialValue

return json ? beforeRead(serialize(json)) : initialValue
const cancel = Symbol("cancel")
const newVal = beforeRead(serialized, cancel)
if (newVal === cancel) return initialValue
return newVal
}

if (!stores[storageType][key]) {
Expand All @@ -91,7 +101,10 @@ export function persisted<T>(key: string, initialValue: T, options?: Options<T>)
onParseError(event.newValue, e)
return
}
const processedVal = beforeRead(newVal)
const cancel = Symbol("cancel")
const processedVal = beforeRead(newVal, cancel)
if (processedVal === cancel) return

set(processedVal)
}
}
Expand Down
68 changes: 54 additions & 14 deletions test/localStorageStore.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ describe('persisted()', () => {
it('publishes updates', () => {
const store = persisted('myKey7', 123)
const values: number[] = []
const unsub = store.subscribe((value : number) => {
const unsub = store.subscribe((value: number) => {
if (value !== undefined) values.push(value)
})
store.set(456)
Expand Down Expand Up @@ -146,7 +146,7 @@ describe('persisted()', () => {
values.push(val)
})

const event = new StorageEvent('storage', {key: 'beforeRead-test', newValue: "2"})
const event = new StorageEvent('storage', { key: 'beforeRead-test', newValue: "2" })
window.dispatchEvent(event)

expect(values).toEqual([0, 4])
Expand All @@ -160,39 +160,79 @@ describe('persisted()', () => {

expect(JSON.parse(localStorage.getItem("beforeWrite-test") as string)).toEqual(4)
})

it("allows to cancel read operation during initialization", () => {
localStorage.setItem("beforeRead-init-cancel", JSON.stringify(2))
const beforeRead = vi.fn(<S extends symbol>(_: any, cancel: S) => cancel)
const store = persisted("beforeRead-init-cancel", 0, { beforeRead })
expect(beforeRead).toHaveBeenCalledOnce()
expect(get(store)).toEqual(0)
})

it("allows to cancel read operation during event handling", () => {
// Will only call beforeRead on init if key exists, so creates key
localStorage.setItem("beforeRead-cancel", JSON.stringify(2))

const beforeRead = vi.fn(<S extends symbol>(_: any, cancel: S) => cancel)
const store = persisted("beforeRead-cancel", 0, { beforeRead })

const values: number[] = []

const unsub = store.subscribe((val: number) => {
values.push(val)
})

const event = new StorageEvent('storage', { key: 'beforeRead-cancel', newValue: "2" })
window.dispatchEvent(event)

expect(beforeRead).toHaveBeenCalledTimes(2)
expect(values).toEqual([0])

unsub()
})

it("allows to cancel write operation", () => {
const beforeWrite = vi.fn(<S extends symbol>(_: number, cancel: S) => cancel)
const store = persisted<number>("beforeWrite-cancel", 0, { beforeWrite })
store.set(2)

expect(JSON.parse(localStorage.getItem("beforeWrite-cancel") as string)).toEqual(null)
expect(get(store)).toEqual(2)
expect(beforeWrite).toHaveBeenCalledOnce()
})
})

describe('handles window.storage event', () => {
type NumberDict = { [key: string] : number }
type NumberDict = { [key: string]: number }

it('sets storage when key matches', () => {
const store = persisted('myKey8', {a: 1})
const store = persisted('myKey8', { a: 1 })
const values: NumberDict[] = []

const unsub = store.subscribe((value: NumberDict) => {
values.push(value)
})

const event = new StorageEvent('storage', {key: 'myKey8', newValue: '{"a": 1, "b": 2}'})
const event = new StorageEvent('storage', { key: 'myKey8', newValue: '{"a": 1, "b": 2}' })
window.dispatchEvent(event)

expect(values).toEqual([{a: 1}, {a: 1, b: 2}])
expect(values).toEqual([{ a: 1 }, { a: 1, b: 2 }])

unsub()
})

it('ignores storages events when value is null', () => {
joshnuss marked this conversation as resolved.
Show resolved Hide resolved
const store = persisted('myKey9', {a: 1})
const store = persisted('myKey9', { a: 1 })
const values: NumberDict[] = []

const unsub = store.subscribe((value: NumberDict) => {
values.push(value)
})

const event = new StorageEvent('storage', {key: 'myKey9', newValue: null})
const event = new StorageEvent('storage', { key: 'myKey9', newValue: null })
window.dispatchEvent(event)

expect(values).toEqual([{a: 1}])
expect(values).toEqual([{ a: 1 }])

unsub()
})
Expand All @@ -205,7 +245,7 @@ describe('persisted()', () => {
values.push(value)
})

const event = new StorageEvent('storage', {key: 'unknownKey', newValue: '2'})
const event = new StorageEvent('storage', { key: 'unknownKey', newValue: '2' })
window.dispatchEvent(event)

expect(values).toEqual([1])
Expand All @@ -219,7 +259,7 @@ describe('persisted()', () => {
const store = persisted('myKeyb', 1)
const values: number[] = []

const event = new StorageEvent('storage', {key: 'myKeyb', newValue: '2'})
const event = new StorageEvent('storage', { key: 'myKeyb', newValue: '2' })
window.dispatchEvent(event)

const unsub = store.subscribe((value: number) => {
Expand All @@ -239,7 +279,7 @@ describe('persisted()', () => {
values.push(value)
})

const event = new StorageEvent('storage', {key: 'myKey10', newValue: '2'})
const event = new StorageEvent('storage', { key: 'myKey10', newValue: '2' })
window.dispatchEvent(event)

expect(values).toEqual([1])
Expand All @@ -255,7 +295,7 @@ describe('persisted()', () => {
values.push(value)
})

const event = new StorageEvent('storage', {key: 'myKey13', newValue: '2'})
const event = new StorageEvent('storage', { key: 'myKey13', newValue: '2' })
window.dispatchEvent(event)

expect(values).toEqual([1])
Expand All @@ -278,7 +318,7 @@ describe('persisted()', () => {
store.update(d => d.add(4))

expect(value).toEqual(testSet)
expect(localStorage.myKey11).toEqual(serializer.stringify(new Set([1,2,3,4])))
expect(localStorage.myKey11).toEqual(serializer.stringify(new Set([1, 2, 3, 4])))
})

it('lets you switch storage type', () => {
Expand Down