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

fix: Move constructor initializing of props to componentWillMount #506

Merged
merged 4 commits into from Mar 7, 2017

Conversation

Projects
None yet
3 participants
@drager
Copy link
Contributor

drager commented Mar 6, 2017

React outputs setState warnings (constructor side-effects are an anti-pattern), because of that initializing of props in the constructor and by moving that code to componentWillMount fixes that.

fix: Move constructor initializing of props to componentWillMount
React outputs setState warnings because of that initializing of props in the constructor and by moving that code to componentWillMount fixes that
@calebmer

This comment has been minimized.

Copy link
Contributor

calebmer commented Mar 6, 2017

Hmm…we don’t set state anywhere in that code 😣

Could you make a small reproduction with react-apollo-error-template so I can see where the error is being triggered?

@calebmer

This comment has been minimized.

Copy link
Contributor

calebmer commented Mar 6, 2017

It looks like this error was reproduced in this issue: #509

@drager can you confirm this is the same issue?

I want to take some time to look at the reproduction and understand what is really going on to see if we can get a more specific fix.

calebmer added some commits Mar 6, 2017

@calebmer

This comment has been minimized.

Copy link
Contributor

calebmer commented Mar 6, 2017

After more investigation in #509 it looks like this does indeed fix the problem. I cleaned it up a little, @drager could you add a changelog entry and then we can merge? 😊

@drager

This comment has been minimized.

Copy link
Contributor Author

drager commented Mar 7, 2017

@calebmer: Thanks, sorry for the late answer but great that #509 could confirm this issue. Will add a changelog entry as soon as possible. :)

@calebmer calebmer merged commit b52c3b7 into apollographql:master Mar 7, 2017

3 checks passed

CLA Author has signed the Meteor CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 93.25%
Details
@morgante

This comment has been minimized.

Copy link

morgante commented Mar 7, 2017

When can we expect this to be released? Has anyone found a workaround for now?

@calebmer

This comment has been minimized.

Copy link
Contributor

calebmer commented Mar 7, 2017

Released in 0.13.3 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment