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

Immutable root state #37

Closed
fabriziomoscon opened this issue Sep 25, 2017 · 5 comments
Closed

Immutable root state #37

fabriziomoscon opened this issue Sep 25, 2017 · 5 comments

Comments

@fabriziomoscon
Copy link

Hi @sorodrigo,
thanks a lot for opening and maintaining this repository.
A couple of months ago I opened this PR: redux-offline#99
to allow the use of immutable root state with redux-offline.
Since then we have been successfully using the fork with our immutable root state.
I wanted to check with you whether the same PR could be merged in this fork if I create it and rebase it in top of the current improvements.

Thanks

@sorodrigo
Copy link
Owner

Sounds interesting! I skimmed through the other PR and through redux-offline-immutable-config. The only thing I'm not thrilled about, is that even if one's not dealing with an immutable store, there's a slight overhead introduced to the core because of the lenses.
Now, don't get me wrong, immutable support is a nice thing to have! I'd like to hear more from @wacii given that he's been updating the enhacer and other internals recently.

@sorodrigo
Copy link
Owner

On another note, the redux-offline org seems like a good place where your config package could live in.

@wacii
Copy link
Collaborator

wacii commented Sep 27, 2017

It looks good. About as compact a change as I think possible.

Injecting autoRehydrate() is something we should be doing already. It doesn't make a whole lot of sense to inject persist() but not autoRehydrate() as we do now.

As for the lens, you are either going to need such an abstraction or to build two versions of the project. I prefer adding an abstraction.

@wacii
Copy link
Collaborator

wacii commented Sep 27, 2017

That said I don't understand the benefit of making the root object immutable.

@fabriziomoscon
Copy link
Author

Thanks for all comments.
The original idea was to make 100% non breaking changes. For that we set defaults everywhere possible.
To allow the use of immutable.js we had to inject:

  • persistCallback
  • persistAutoRehydrate
  • offlineStateLens
    but defaults will allow user to ignore them.

When using immutable setting the entire redux state as immutable is a best practice:
http://redux.js.org/docs/recipes/UsingImmutableJS.html#make-your-entire-redux-state-tree-an-immutablejs-object

redux-offline-immutable-config provides default configuration for users using immutable.js, we created a separate repo so that only immutable users need to install the extra bits.

I will go through the rebase and see if there are any incompatibilities

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

No branches or pull requests

3 participants