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

Move ref context binding to fix object freezing middleware #733

Merged
merged 1 commit into from
Jul 6, 2017

Conversation

gpoitch
Copy link
Contributor

@gpoitch gpoitch commented Jul 6, 2017

Closes #708

Middleware that uses Object.freeze such as redux-freeze started throwing an error somewhere between versions 5.0.0...5.0.5:

Uncaught TypeError: Cannot assign to read only property 'setWrappedInstance' of object '#<Connect>'

This is because Connect instances are being mutated here: https://github.com/reactjs/react-redux/blob/2726c59fa1e5929659067df2088c7c6e0d736f3a/src/components/connectAdvanced.js#L124

This PR moves the context binding of the ref function to a lazy assignment and doesn't mutate the original function. IMO the intention of the code is clearer this way as well.

@jimbolla
Copy link
Contributor

jimbolla commented Jul 6, 2017

Except now it's creating a new bound function every render. I don't think we want to do this.

@timdorr
Copy link
Member

timdorr commented Jul 6, 2017

LGTM. Thanks!

@timdorr timdorr merged commit 2118893 into reduxjs:master Jul 6, 2017
@timdorr
Copy link
Member

timdorr commented Jul 6, 2017

Oh crap, I didn't follow that code all the way through.

Binding in the constructor is super-common. I have a hard time believing redux-freeze can't handle this. I'm not sure why it's touching a component instance anyways. That was never made clear to me.

@gpoitch
Copy link
Contributor Author

gpoitch commented Jul 6, 2017

Ok. Here you go: #735
It's nothing directly to do with redux-freeze, rather that's how Object.freeze works.

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.

3 participants