fix(middleware/persist): ensure `persist` does not drop updates in `onRehydrateStorage` when using a synchronous storage API #1689

coffeebeats · 2023-03-14T05:56:22Z

Related Issues or Discussions

Summary

See #1688 for a detailed description of the problem. This PR resolves the issue by ensuring stateFromStorage gets updated to the most recent value. I also added tests verifying this update pattern works for both synchronous and asynchronous storage APIs.

One note for reviewers: I added a comment describing how, in the asynchronous case, it's possible that the state passed to postRehydrationCallback is not the most up-to-date state. This would occur if there was a state update between callbacks in the hydration promise chain. Please let me know if I should leave the comment as-is, make the change, or simply remove the comment.

Check List

yarn run prettier for formatting code and docs

…ateStorage'

…rwritten with sync storage reads

…andled correctly

codesandbox-ci · 2023-03-14T05:56:49Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 0698a73:

Sandbox	Source
React	Configuration
React Typescript	Configuration
React Browserify	Configuration
React Snowpack	Configuration
React Parcel	Configuration
@pavlobu/zustand demo	Configuration
zustand-4.3.X-bug (forked)	Issue #1688

dai-shi

looks correct.
can anyone else review too?

dai-shi · 2023-03-14T12:02:49Z

tests/persistAsync.test.tsx

+        {
+          name: 'test-storage',
+          storage: createJSONStorage(() => storage),
+          onRehydrateStorage: () => (s) => s?.inc(),


Do we need ??

We do - the type signature for the method returned by onRehydrateStorage is (state?: S, error?: unknown) => void, so state might not be defined. If somehow s is undefined here, the test will fail as expected.

sewera

Looks correct also to me.

sewera · 2023-03-14T16:54:21Z

It's a tricky topic that will not have to be rediscovered with this comment, so I'd leave it as-is.

coffeebeats · 2023-03-14T18:36:21Z

Does this also need to be fixed for the old implementation? I originally assumed no, but after looking it some more it seems like it probably should be. If that's the case, are there separate tests for the old implementation?

dai-shi · 2023-03-15T02:32:42Z

Does this also need to be fixed for the old implementation?

My plan is to leave the old implementation unchanged. People shouldn't use the deprecated one moving forward.

tests/persistSync.test.tsx

tests/persistAsync.test.tsx

coffeebeats added 3 commits March 13, 2023 17:25

document a potential issue with stale state being passed to 'onRehydr…

fac1ec3

…ateStorage'

fix an issue where state updates during 'onRehydrateStorage' were ove…

d1fdc8e

…rwritten with sync storage reads

add tests ensuring updates during the 'onRehydrateStorage' hook are h…

10b66d5

…andled correctly

dai-shi reviewed Mar 14, 2023

View reviewed changes

sewera approved these changes Mar 14, 2023

View reviewed changes

dai-shi approved these changes Mar 31, 2023

View reviewed changes

dai-shi added this to the v4.3.7 milestone Mar 31, 2023

Merge branch 'main' into fix/sync-storage-update-in-hydrate-cb

24ead64

dai-shi reviewed Mar 31, 2023

View reviewed changes

tests/persistSync.test.tsx Outdated Show resolved Hide resolved

Update tests/persistSync.test.tsx

6ccba38

dai-shi reviewed Mar 31, 2023

View reviewed changes

tests/persistSync.test.tsx Show resolved Hide resolved

dai-shi reviewed Mar 31, 2023

View reviewed changes

tests/persistAsync.test.tsx Show resolved Hide resolved

Apply suggestions from code review

0698a73

dai-shi merged commit 309c672 into pmndrs:main Mar 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(middleware/persist): ensure `persist` does not drop updates in `onRehydrateStorage` when using a synchronous storage API #1689

fix(middleware/persist): ensure `persist` does not drop updates in `onRehydrateStorage` when using a synchronous storage API #1689

coffeebeats commented Mar 14, 2023

codesandbox-ci bot commented Mar 14, 2023 •

edited

Loading

dai-shi left a comment

dai-shi Mar 14, 2023

coffeebeats Mar 14, 2023

sewera left a comment

sewera commented Mar 14, 2023

coffeebeats commented Mar 14, 2023

dai-shi commented Mar 15, 2023

fix(middleware/persist): ensure persist does not drop updates in onRehydrateStorage when using a synchronous storage API #1689

fix(middleware/persist): ensure persist does not drop updates in onRehydrateStorage when using a synchronous storage API #1689

Conversation

coffeebeats commented Mar 14, 2023

Related Issues or Discussions

Summary

Check List

codesandbox-ci bot commented Mar 14, 2023 • edited Loading

dai-shi left a comment

Choose a reason for hiding this comment

dai-shi Mar 14, 2023

Choose a reason for hiding this comment

coffeebeats Mar 14, 2023

Choose a reason for hiding this comment

sewera left a comment

Choose a reason for hiding this comment

sewera commented Mar 14, 2023

coffeebeats commented Mar 14, 2023

dai-shi commented Mar 15, 2023

fix(middleware/persist): ensure `persist` does not drop updates in `onRehydrateStorage` when using a synchronous storage API #1689

fix(middleware/persist): ensure `persist` does not drop updates in `onRehydrateStorage` when using a synchronous storage API #1689

codesandbox-ci bot commented Mar 14, 2023 •

edited

Loading