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 6 commits
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
2 changes: 2 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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*/},
beforeWrite: (value) => {/* change value after writing to store, but before writing return value to local storage*/},
})
```

Expand Down
44 changes: 32 additions & 12 deletions index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,20 +14,22 @@ const stores: Stores = {
session: {}
}

export interface Serializer<T> {
parse(text: string): T
stringify(object: T): string
export interface Serializer {
parse(text: string): any
stringify(object: any): string
}

export type StorageType = 'local' | 'session'

export interface Options<T> {
serializer?: Serializer<T>
serializer?: Serializer
storage?: StorageType,
syncTabs?: boolean,
onError?: (e: unknown) => void
onWriteError?: (e: unknown) => void
onParseError?: (newValue: string | null, e: unknown) => void
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 All @@ -47,45 +49,63 @@ export function persisted<T>(key: string, initialValue: T, options?: Options<T>)
const syncTabs = options?.syncTabs ?? true
const onWriteError = options?.onWriteError ?? options?.onError ?? ((e) => console.error(`Error when writing value from persisted store "${key}" to ${storageType}`, e))
const onParseError = options?.onParseError ?? ((newVal, e) => console.error(`Error when parsing ${newVal ? '"' + newVal + '"' : "value"} from persisted store "${key}"`, e))

const beforeRead = options?.beforeRead ?? ((val) => val as T)
const beforeWrite = options?.beforeWrite ?? ((val) => val as any)

const browser = typeof (window) !== 'undefined' && typeof (document) !== 'undefined'
const storage = browser ? getStorage(storageType) : null

function updateStorage(key: string, value: T) {
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(value))
storage?.setItem(key, serializer.stringify(newVal))
} catch (e) {
onWriteError(e)
}
}

function maybeLoadInitial(): T {
const json = storage?.getItem(key)

if (json) {
function serialize(json: any) {
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

try {
return <T>serializer.parse(json)
} catch (e) {
onParseError(json, e)
}
}
const json = storage?.getItem(key)
if (json == null) return initialValue

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

const cancel = Symbol("cancel")
const newVal = beforeRead(serialized, cancel)
if (newVal === cancel) return initialValue
return newVal
}

if (!stores[storageType][key]) {
const initial = maybeLoadInitial()
const store = internal(initial, (set) => {
if (browser && storageType == 'local' && syncTabs) {
const handleStorage = (event: StorageEvent) => {
if (event.key === key) {
if (event.key === key && event.newValue) {
let newVal: any
try {
newVal = event.newValue ? serializer.parse(event.newValue) : null
newVal = serializer.parse(event.newValue)
} catch (e) {
onParseError(event.newValue, e)
return
}
set(newVal)
const cancel = Symbol("cancel")
const processedVal = beforeRead(newVal, cancel)
if (processedVal === cancel) return

set(processedVal)
}
}

Expand Down
98 changes: 84 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 @@ -132,37 +132,107 @@ describe('persisted()', () => {
unsub2()
})

describe("beforeRead and beforeWrite", () => {
it("allows modifying initial value before reading", () => {
localStorage.setItem("beforeRead-init-test", JSON.stringify(2))
const store = persisted("beforeRead-init-test", 0, { beforeRead: (v) => v * 2 })
expect(get(store)).toEqual(4)
})
it("allows modifying value before reading upon event", () => {
const store = persisted("beforeRead-test", 0, { beforeRead: (v) => v * 2 })
const values: number[] = []

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

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

expect(values).toEqual([0, 4])

unsub()
})

it("allows modifying value before writing", () => {
const store = persisted("beforeWrite-test", 0, { beforeWrite: (v) => v * 2 })
store.set(2)

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('sets store to null when value is null', () => {
const store = persisted('myKey9', {a: 1})
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 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}, null])
expect(values).toEqual([{ a: 1 }])

unsub()
})
Expand All @@ -175,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 @@ -189,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 @@ -209,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 @@ -225,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 @@ -248,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