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

Added react-router + redux-localstorage #14

Merged
merged 15 commits into from
Feb 1, 2017
Merged

Added react-router + redux-localstorage #14

merged 15 commits into from
Feb 1, 2017

Conversation

pronebird
Copy link
Collaborator

@pronebird pronebird commented Feb 1, 2017

Hi,

I've added react-router and redux-localstorage.

Local storage was primarily added to workaround the issue of preserving redux state between reloads that are currently done via browser-sync.

Thoughts/comments?

p.s: will squash before merge.

@jschr
Copy link
Owner

jschr commented Feb 1, 2017

Changes look good, merge at will! 👍

@pronebird
Copy link
Collaborator Author

pronebird commented Feb 1, 2017

@jschr tried redux-persist but had a warning when calling autoRehydrate on reload, described here: michaelcontento/redux-storage#121

I don't have time to investigate this and I think this can be tackled in future, but so far redux-localstorage works fine and requires minimum configuration.

@pronebird pronebird merged commit eb6f72e into master Feb 1, 2017
@jschr
Copy link
Owner

jschr commented Feb 1, 2017

I haven't used redux-storage before. Just curious, Is that the one that errors or https://github.com/rt2zz/redux-persist?

@pronebird
Copy link
Collaborator Author

@jschr yeah I haven't used it either, but I've got the same error when using redux-persist

@jschr
Copy link
Owner

jschr commented Feb 1, 2017

Ah ok, good to know!

I've run into similar errors using redux-persist in a chrome packaged app too.

@pronebird
Copy link
Collaborator Author

pronebird commented Feb 15, 2017

I think the error we experienced was related to autoHydrate when the same URL was pushed twice and apparently react router does not like that.

redux-persist probably pushes state to reducers dynamically after reading local storage. Therefore if initial path was / and the path loaded from state is / then we get a funny warning.

I have an assumption that redux-localstorage simply swaps entire state from local storage without telling anyone :) which should be alright as long as you handle time travelling in your app (i.e expired sessions etc.. but that's something you have to handle anyway).

On the other side, this produces certain side effects when used with syncHistoryWithStore from react-router-redux which gets confused regarding where user comes from and where it goes to. But that's just because of a wrong assumption in their code.

History object should be always consulted regarding current location but I don't see that happening when debugging react-router-redux on cold restart.

We could just swap redux-localstorage to redux-persist and think the problem is solved, however I noticed a significant delay between initial load and autoHydrate which is annoying because I could see the entire view rendered broken for a second.

If I am not entirely crazy this should be legit issue: reactjs/react-router-redux#534

I am hoping to fix that in boilerplate too once we figure out what exactly going on.

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.

2 participants