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

(persistStore): add timeout option #702

Closed
wants to merge 2 commits into from
Closed

(persistStore): add timeout option #702

wants to merge 2 commits into from

Conversation

rt2zz
Copy link
Owner

@rt2zz rt2zz commented Feb 3, 2018

if timeout is exceeded, rehydrate will complete with an error. In the past it would simply hang indefinitely.

timeout is default undefined -> meaning no effect.

questions:

  1. this adds ~120 Bytes - is this worth it?
  2. should timeout be defaulted to something large but reasonable like 5s?
  3. rather than be a part of redux-persist should timeout be a feature of the storage adapter?

@samhunta
Copy link

samhunta commented Feb 4, 2018

Re:

  1. Worth it, would have saved me time debugging this AsyncStorage bug
  2. A timeout of even less (3s) would be nice for my use cases.
  3. I think it fits here well since the functionality doesn't change depending on the storage adapter

@rt2zz
Copy link
Owner Author

rt2zz commented Feb 5, 2018

👍 ok I think I agree, lets add this and default the timeout. I still want to evaluate if timeout would better live in persistReducer or here (persistStore). Will look to merge this is something comparable this week.

@JulianKingman
Copy link

I like in persistReducer, since other config goes in there as well. However, I think it would be handy to pass the persist status to the persistStore callback (i.e. error or success), that way we can handle timeout cases. I like a default time, because if it's taking too long, something is likely wrong and the dev should know about it.

@rimzici
Copy link

rimzici commented Feb 15, 2018

Any Updates on this?

@rt2zz
Copy link
Owner Author

rt2zz commented Feb 16, 2018

this landed via #713 - applied to persistReducer instead of persistStore 👍

@rt2zz rt2zz closed this Feb 16, 2018
@rt2zz rt2zz deleted the timeout branch February 16, 2018 07:25
@rimzici
Copy link

rimzici commented Feb 17, 2018

I see, thank you.
A small confirmation, by this fix the issue of app being infinitely hung is solved, but the data is lost after the default timeout of 5000ms ?

@rt2zz
Copy link
Owner Author

rt2zz commented Feb 19, 2018

yes the data will be lost if timeout is ever reached

@nico1510
Copy link

Hmm but is that really the better alternative ? I mean for my use case I would rather have the user restart the app if it doesn't render than losing all the data and continuing... Can we avoid this new logic by e.g. setting the timeout to -1 in the config or should I just pass a large number in there ?

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.

5 participants