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

Localize component doesn't work with redux-immutable #49

Closed
jamiedust opened this issue Jan 29, 2018 · 9 comments
Closed

Localize component doesn't work with redux-immutable #49

jamiedust opened this issue Jan 29, 2018 · 9 comments

Comments

@jamiedust
Copy link
Contributor

Hi there, thanks for a great project, we are using this in our app.

The higher order component Localize does not work when using redux-immutable, this is because the mapStateToProps function expects to find the slice at state[slice], whereas when using the redux-immutable combineReducers it saves the state as a Map.

This can be worked around, for example in our app we have used the normal combineReducers so that our state itself is a normal object, but our slices are immutable Maps. However this is not ideal because it breaks some conventions across our code base. Also it could put people off when trying to implement what is otherwise a great translation solution.

I suggest that a check is made and if the state object is a Map then state.get(slice) is called, I have tested this in my app and it works fine.

@jamiedust
Copy link
Contributor Author

I have submitted a pull request #50 which solves the issue described above.

@ryandrewjohnson
Copy link
Owner

Thanks for the PR. I will have a closer look when I get some time, but in the meantime if you used plain old connect instead of the Localize HOC like it's shown here would that get around the issue you're having?

@jamiedust
Copy link
Contributor Author

Yes it does solve the problem, I wanted to avoid using connect in our app as we have a lot of smaller components. Although I was going to suggest that devs prefer passing translations as props rather than using Localize everywhere, as per your docs.

I would still rather use Localize HOC as its a bit cleaner and I can see other devs abusing connect if they see it being used in places where we otherwise wouldn't.

Its a fair point though and something I will consider as our project progresses.

Thanks.

@ryandrewjohnson
Copy link
Owner

One thing to keep in mind is that Localize is just a facade around connect. So connect can still be abused by overusing Localize as well. You got the right idea though with passing translations down to child components.

@ryandrewjohnson
Copy link
Owner

Confirmed fix from #50. This is available in v2.15.0.

@jamiedust
Copy link
Contributor Author

Great thanks!

@jamiedust
Copy link
Contributor Author

Hi, I would like to reopen this issue, it appears that my fix does not in fact work after all, I misunderstood quite what an immutable Map was, and also didn't test this on my own app properly, as it was still in early development, apologies for that.

In order to check this correctly we need to import the Immutable Map object and do Map.isMap(). I have tested this in my own app using Immutable for the root state, with the localize HOC and this works as expected.

@jamiedust
Copy link
Contributor Author

Pull request submitted. #57

Thanks for your time and sorry about this!

@ryandrewjohnson
Copy link
Owner

Fixed in #58 and released in v2.16.0

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

2 participants