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.

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

Migrating Async/Await to Promise #403

Merged
merged 35 commits into from
Jun 5, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
0dc8df0
setItem migrated from asyn/await to promise
byteab May 25, 2021
e1d6539
setItem is more cleaner now
byteab May 26, 2021
64ec1a5
rehydrate on initial state migrated to promise 🔥
byteab May 26, 2021
7bfb38e
a tiny bug fixed in middleware
byteab May 26, 2021
658e9c9
.size-snapshot.json update
byteab May 26, 2021
d80f3f0
don't invoke serialize when storage is undefined
byteab May 27, 2021
b6c7953
a makeThenable() used to mix sync/async thens
byteab May 27, 2021
055dd02
toThenable() API finalized
byteab May 29, 2021
544b96d
size snapshots updated
byteab May 29, 2021
3935b50
Add tests for sync storage
barbogast May 29, 2021
a6f4f1f
Fix loading the state from localStorage
barbogast May 30, 2021
d213ae8
Merge pull request #2 from barbogast/fix-loading-state
byteab May 30, 2021
fc4a4ab
new snapshot
byteab May 30, 2021
bc9ff93
types ✨ added to `toThenable()` 🎉 🎉
byteab May 31, 2021
cb704bc
new size snapshot
byteab May 31, 2021
fc9fdd1
Update src/middleware.ts
byteab May 31, 2021
8e91e5b
remaining parts also migrated to `toThenable()`
byteab May 31, 2021
76e1dd0
a clear explanation of sync storage workaround
byteab May 31, 2021
bc17cd7
fix some inconsistencies
byteab May 31, 2021
4388753
some inconsistency fixed
byteab May 31, 2021
6de92a2
useless IIFE removed
byteab May 31, 2021
8dda53e
some bugs and types fixed
byteab Jun 1, 2021
fde0b7a
stateFromStorage renamed to stateFromStorageInSync
byteab Jun 2, 2021
5e06a34
Wrap tests into describe()
barbogast Jun 1, 2021
0f7dca9
Make sure that test for async persist() actually tests an async middl…
barbogast Jun 1, 2021
a880363
Merge branch 'address-review-comments' into async-await-fix
byteab Jun 2, 2021
372a192
size snap shot updated
byteab Jun 2, 2021
58430ef
Correctly handle version discrepancies with missing migrate functions
barbogast Jun 2, 2021
998f038
Update src/middleware.ts
byteab Jun 2, 2021
9cea260
Fix test
barbogast Jun 3, 2021
f39f077
Merge pull request #4 from barbogast/handle-missing-migrate
byteab Jun 3, 2021
cd36c95
chages addressed in reviews applied
byteab Jun 4, 2021
2e75b6e
Merge branch 'master' into async-await-fix
dai-shi Jun 4, 2021
f8f586d
chore: refactor
dai-shi Jun 4, 2021
473c16a
fix: setItem can throw error in sync
dai-shi Jun 4, 2021
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
6 changes: 3 additions & 3 deletions .size-snapshot.json
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,9 @@
}
},
"middleware.js": {
"bundled": 5547,
"minified": 2945,
"gzipped": 1293,
"bundled": 6548,
"minified": 3299,
"gzipped": 1388,
"treeshaked": {
"rollup": {
"code": 0,
Expand Down
120 changes: 94 additions & 26 deletions src/middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -204,15 +204,52 @@ type PersistOptions<S> = {
migrate?: (persistedState: any, version: number) => S | Promise<S>
}
byteab marked this conversation as resolved.
Show resolved Hide resolved

interface Thenable<Value> {
then<V>(
onFulfilled: (value: Value) => V | Promise<V> | Thenable<V>
): Thenable<V>
catch<V>(
onRejected: (reason: Error) => V | Promise<V> | Thenable<V>
): Thenable<V>
}

const toThenable = <Result, Input>(
fn: (input: Input) => Result | Promise<Result> | Thenable<Result>
) => (input: Input): Thenable<Result> => {
try {
const result = fn(input)
if (result instanceof Promise) {
return result as Thenable<Result>
}
return {
then(onFulfilled) {
return toThenable(onFulfilled)(result as Result)
},
catch(_onRejected) {
return this as Thenable<any>
},
}
} catch (e) {
return {
then(_onFulfilled) {
return this as Thenable<any>
},
catch(onRejected) {
return toThenable(onRejected)(e)
},
}
}
}

export const persist = <S extends State>(
config: StateCreator<S>,
options: PersistOptions<S>
) => (set: SetState<S>, get: GetState<S>, api: StoreApi<S>): S => {
const {
name,
getStorage = () => localStorage,
serialize = JSON.stringify,
deserialize = JSON.parse,
serialize = JSON.stringify as (state: StorageValue<S>) => string,
deserialize = JSON.parse as (str: string) => StorageValue<S>,
blacklist,
whitelist,
onRehydrateStorage,
Expand Down Expand Up @@ -241,7 +278,9 @@ export const persist = <S extends State>(
)
}

const setItem = async () => {
const thenableSerialize = toThenable(serialize)

const setItem = (): Thenable<void> => {
const state = { ...get() }

if (whitelist) {
Expand All @@ -253,7 +292,18 @@ export const persist = <S extends State>(
blacklist.forEach((key) => delete state[key])
}

return storage?.setItem(name, await serialize({ state, version }))
let errorInSync: Error | undefined
const thenable = thenableSerialize({ state, version })
.then((serializedValue) =>
(storage as StateStorage).setItem(name, serializedValue)
)
.catch((e) => {
errorInSync = e
})
if (errorInSync) {
throw errorInSync
}
return thenable
}

const savedSetState = api.setState
Expand All @@ -264,43 +314,61 @@ export const persist = <S extends State>(
}

// rehydrate initial state with existing stored state
;(async () => {
const postRehydrationCallback = onRehydrateStorage?.(get()) || undefined

try {
const storageValue = await storage.getItem(name)

// a workaround to solve the issue of not storing rehydrated state in sync storage
// the set(state) value would be later overridden with initial state by create()
// to avoid this, we merge the state from localStorage into the initial state.
let stateFromStorageInSync: S | undefined
const postRehydrationCallback = onRehydrateStorage?.(get()) || undefined
// bind is used to avoid `TypeError: Illegal invocation` error
toThenable(storage.getItem.bind(storage))(name)
byteab marked this conversation as resolved.
Show resolved Hide resolved
.then((storageValue) => {
if (storageValue) {
const deserializedStorageValue = await deserialize(storageValue)

// if versions mismatch, run migration
return deserialize(storageValue)
}
})
.then((deserializedStorageValue) => {
if (deserializedStorageValue) {
if (deserializedStorageValue.version !== version) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (deserializedStorageValue.version !== version) {
if ((deserializedStorageValue.version ?? false) && deserializedStorageValue.version !== version) {

see #409

Copy link
Member

Choose a reason for hiding this comment

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

removing ?? false just works?

Copy link
Collaborator

Choose a reason for hiding this comment

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

not if the version is 0

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, my bad. This doesn't even work if the version is 0. So maybe something like: typeof deserializedStorageValue.version === "number" && deserializedStorageValue.version !== version?

Copy link
Member

Choose a reason for hiding this comment

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

StorageValue<S> requires version to be number so we can even throw an error if typeof ... !== "number".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, that wouldn't permit to store a primitive value as in #409
But maybe there is a reason not to allow that?

Copy link
Member

Choose a reason for hiding this comment

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

well, if we want to allow non-version store, we should also change the StorageValue<S> type. I'm fine with either way.

Copy link
Member

Choose a reason for hiding this comment

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

Should we tackle it as a separate PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, let's merge this first

const migratedState = await migrate?.(
deserializedStorageValue.state,
deserializedStorageValue.version
)
if (migratedState) {
set(migratedState)
await setItem()
if (migrate) {
return migrate(
deserializedStorageValue.state,
deserializedStorageValue.version
)
}
console.error(
`State loaded from storage couldn't be migrated since no migrate function was provided`
)
} else {
stateFromStorageInSync = deserializedStorageValue.state
set(deserializedStorageValue.state)
}
}
} catch (e) {
})
AnatoleLucet marked this conversation as resolved.
Show resolved Hide resolved
.then((migratedState) => {
if (migratedState) {
stateFromStorageInSync = migratedState as S
set(migratedState as PartialState<S, keyof S>)
return setItem()
dai-shi marked this conversation as resolved.
Show resolved Hide resolved
}
})
.then(() => {
postRehydrationCallback?.(get(), undefined)
})
.catch((e: Error) => {
postRehydrationCallback?.(undefined, e)
return
}

postRehydrationCallback?.(get(), undefined)
})()
})

return config(
const configResult = config(
(...args) => {
set(...args)
void setItem()
},
get,
api
)

return stateFromStorageInSync
? { ...configResult, ...stateFromStorageInSync }
: configResult
}
2 changes: 1 addition & 1 deletion tests/persist.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ it('can migrate persisted state', async () => {
name: 'test-storage',
version: 13,
getStorage: () => storage,
migrate: (state, version) => {
migrate: async (state, version) => {
migrateCallCount++
expect(state.count).toBe(42)
expect(version).toBe(12)
Expand Down
201 changes: 201 additions & 0 deletions tests/persistSync.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,201 @@
import create from '../src/index'
import { persist } from '../src/middleware'

byteab marked this conversation as resolved.
Show resolved Hide resolved
const consoleError = console.error
afterEach(() => {
console.error = consoleError
Copy link
Collaborator

@AnatoleLucet AnatoleLucet May 31, 2021

Choose a reason for hiding this comment

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

What is this for? I'm not sure to understand since we're not reassigning console.error, and you're neither mocking it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did copy that from persistent.test.tsx and not verify that it's needed, good catch. I can be removed, right? Might make sense to remove it from persistent.test.tsx as well, I guess.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually this concerns all test files. It's not apparent to me why console.error gets mocked. Does zustand write to console.error, and the mocks where once verifying that? Address in a separate PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@barbogast

Actually this concerns all test files.

Totally. They are not well maintained, I'd say... 😅 Happy to take PRs for improvements. Please address in a separate PR.

})

describe('persist middleware with sync configuration', () => {
it('can rehydrate state', () => {
let postRehydrationCallbackCallCount = 0

const storage = {
getItem: (name: string) =>
JSON.stringify({
state: { count: 42, name },
version: 0,
}),
setItem: () => {},
}

const useStore = create(
persist(
() => ({
count: 0,
name: 'empty',
}),
{
name: 'test-storage',
getStorage: () => storage,
onRehydrateStorage: () => (state, error) => {
postRehydrationCallbackCallCount++
expect(error).toBeUndefined()
expect(state?.count).toBe(42)
expect(state?.name).toBe('test-storage')
},
}
)
)

expect(useStore.getState()).toEqual({ count: 42, name: 'test-storage' })
expect(postRehydrationCallbackCallCount).toBe(1)
})

it('can throw rehydrate error', () => {
let postRehydrationCallbackCallCount = 0

const storage = {
getItem: () => {
throw new Error('getItem error')
},
setItem: () => {},
}

create(
persist(() => ({ count: 0 }), {
name: 'test-storage',
getStorage: () => storage,
onRehydrateStorage: () => (_, e) => {
postRehydrationCallbackCallCount++
expect(e?.message).toBe('getItem error')
},
})
)

expect(postRehydrationCallbackCallCount).toBe(1)
})

it('can persist state', () => {
let setItemCallCount = 0

const storage = {
getItem: () => null,
setItem: (name: string, value: string) => {
setItemCallCount++
expect(name).toBe('test-storage')
expect(value).toBe(
JSON.stringify({
state: { count: 42 },
version: 0,
})
)
},
}

const useStore = create(
persist(() => ({ count: 0 }), {
name: 'test-storage',
getStorage: () => storage,
onRehydrateStorage: () => (_, error) => {
expect(error).toBeUndefined()
},
})
)

expect(useStore.getState()).toEqual({ count: 0 })
useStore.setState({ count: 42 })
expect(useStore.getState()).toEqual({ count: 42 })
expect(setItemCallCount).toBe(1)
})

it('can migrate persisted state', () => {
let migrateCallCount = 0
let setItemCallCount = 0

const storage = {
getItem: () =>
JSON.stringify({
state: { count: 42 },
version: 12,
}),
setItem: (_: string, value: string) => {
setItemCallCount++
expect(value).toBe(
JSON.stringify({
state: { count: 99 },
version: 13,
})
)
},
}

const useStore = create(
persist(() => ({ count: 0 }), {
name: 'test-storage',
version: 13,
getStorage: () => storage,
onRehydrateStorage: () => (_, error) => {
expect(error).toBeUndefined()
},
migrate: (state, version) => {
migrateCallCount++
expect(state.count).toBe(42)
expect(version).toBe(12)
return { count: 99 }
},
})
)

expect(useStore.getState()).toEqual({ count: 99 })
expect(migrateCallCount).toBe(1)
expect(setItemCallCount).toBe(1)
})

it.only('can correclty handle a missing migrate function', () => {
console.error = jest.fn()
const storage = {
getItem: () =>
JSON.stringify({
state: { count: 42 },
version: 12,
}),
setItem: (_: string, value: string) => {},
}

const useStore = create(
persist(() => ({ count: 0 }), {
name: 'test-storage',
version: 13,
getStorage: () => storage,
onRehydrateStorage: () => (_, error) => {
expect(error).toBeUndefined()
},
})
)

expect(useStore.getState()).toEqual({ count: 0 })
expect(console.error).toHaveBeenCalled()
})

it('can throw migrate error', () => {
let postRehydrationCallbackCallCount = 0

const storage = {
getItem: () =>
JSON.stringify({
state: {},
version: 12,
}),
setItem: () => {},
}

const useStore = create(
persist(() => ({ count: 0 }), {
name: 'test-storage',
version: 13,
getStorage: () => storage,
migrate: () => {
throw new Error('migrate error')
},
onRehydrateStorage: () => (_, e) => {
postRehydrationCallbackCallCount++
expect(e?.message).toBe('migrate error')
},
})
)

expect(useStore.getState()).toEqual({ count: 0 })
expect(postRehydrationCallbackCallCount).toBe(1)
})
})