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

Fix bugs and add tests for sync storage #1

Closed

Conversation

barbogast
Copy link

No description provided.

@byteab
Copy link
Owner

byteab commented May 29, 2021

Thank you @barbogast I will check that.
maybe I can pick the test part for now.

@byteab
Copy link
Owner

byteab commented May 29, 2021

after finalizing the toThenable. would you mind to please help on resolving bugs by another PR.
currently tests are also failing for async as well.
if you have time would you please help me to spot why async test is failing.


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

Choose a reason for hiding this comment

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

@barbogast
Copy link
Author

barbogast commented May 29, 2021

would you mind to please help on resolving bugs by another PR.

How about we merge this PR as it is? Then all the relevant code would be in pmndrs#403 and we would be back to one place in which the conversation happens. Working on toThenable should be easier then, because we've got green tests.

@byteab
Copy link
Owner

byteab commented May 29, 2021

would you mind to please help on resolving bugs by another PR.

How about we merge this PR as it is? Then all the relevant code would be in pmndrs#403 and we would be back to one place in which the conversation happens. Working on toThenable should be easier then because we've got green tests.

Hmm, I was thinking to first have a final solution on how to implement toThenable(), and after that, we start bug fixing. because on the new version of toThenable() as provided on gist we handle try catch. and some commits on this PR may not be relevant to merge.
what do you think?

@byteab
Copy link
Owner

byteab commented May 29, 2021

I will cherry-pick the test part for now.

@barbogast
Copy link
Author

I guess the tests are the important part, so yeah, I agree. 7052e64 is also relevant, AFAICS, since it fixes the initial loading of the state from localStorage.

@byteab
Copy link
Owner

byteab commented May 29, 2021

the tests are now available on async-await-fix branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants