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

Make library work with Immutable.js stores #54

Closed
wants to merge 4 commits into from

Conversation

okjulian
Copy link

No description provided.

@okjulian okjulian force-pushed the immutable-store branch 5 times, most recently from 592a01f to ddc9def Compare April 13, 2017 18:15
@jevakallio
Copy link
Collaborator

Thanks for this @JulianMayorga! 🍺

I'll have to test this a bit and check that everything is working smoothly. Will get back to this next week.

@okjulian
Copy link
Author

Thank you @jevakallio for such a useful library. The documentation was super useful to me.

Let me know if anything comes up after your tests!

@unindented
Copy link

I'd be against landing this PR. redux-persist-immutable is huge because of transit-immutable-js and transit-js. It will make your library too big to ship in a browser scenario.

You can already use Immutable.js by passing the necessary persistOptions, and you don't force people who don't care about transit-js to include it in their bundles and serve it to their users.

redux-offline dispatches actions that do not get received by other middlewares, but they do get received by reducers. redux-little-router experienced the same problem, and they solved it by exposing a middleware, as seen in https://github.com/FormidableLabs/redux-little-router/pull/96/files
@migueloller
Copy link
Contributor

@JulianMayorga, try to rebase master? Having the merge commit in the PR makes it very hard to identify the new changes you made (as opposed to all changes that have happened in the project since you forked it, which seems to be quite a bit ago). Also, there are various merge conflicts due to this.

@okjulian
Copy link
Author

Sorry @migueloller! I made some changes to my fork of redux-offline, but I never intended those changes to be accepted by you.

I made them knowing that they won't be compatible with v2 of redux-offline. For example, I reverted my fork to use v1 installation method, and I also unignored lib folder because I did not want to publish my fork.

I think this means that I should close this PR to avoid future confusion. So feel free to close this :D

I would be more than willing to help you guys if you ever want to support immutable states! So please tell me if you want to reopen a discussion regarding Immutable.

Cheers!

@migueloller
Copy link
Contributor

It's up to @jevakallio what happens but he's likely open to adding these features. It's just that it's a bit hard to look at the changes without rebasing master.

@okjulian okjulian mentioned this pull request May 17, 2017
@okjulian
Copy link
Author

okjulian commented May 17, 2017

I think it's better if we start an issue about using Immutable stores, so I created it on #78. Let's continue the discussion over there!

@jevakallio, I am closing this old PR to avoid confusion.

@fabriziomoscon
Copy link
Contributor

I created a PR #99 for this very same issue

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.

None yet

5 participants