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

Fix #6114 - Calling setState inside getChildContext should warn #6121

Merged
merged 1 commit into from Mar 5, 2016

Conversation

Projects
None yet
4 participants
@raineroviir
Copy link

commented Feb 25, 2016

It doesn't pass one of the tests, but I'm curious if this is the right direction or not.

@@ -498,6 +498,7 @@ var ReactCompositeComponentMixin = {
var inst = this._instance;
var childContext = inst.getChildContext && inst.getChildContext();
if (childContext) {
ReactCurrentOwner.processingChildContext = true;

This comment has been minimized.

Copy link
@jimfb

jimfb Feb 25, 2016

Contributor

Probably don't want to attach this to ReactCurrentOwner, since it has nothing to do with owners. But conceptually, yes, we want to set some sort of value when we start a getChildContext.

I think it probably makes sense to use the new devtools api here. You can emit an event when you start/end getting the child context, and keep the state in a devtool instead of tying it into the core. Devtool api details: https://github.com/facebook/react/pull/5590/files

This comment has been minimized.

Copy link
@raineroviir

raineroviir Feb 26, 2016

Author

Are there any examples of the Devtool api used in React code?
Would I be writing my own devtool function or do I use something like ReactDOMInstrumentation.debugTool.onSetValueForProperty(inst, 'processingChildContext', true)

This comment has been minimized.

Copy link
@gaearon

gaearon Feb 26, 2016

Member

Would I be writing my own devtool function or do I use something like ReactDOMInstrumentation.debugTool.onSetValueForProperty(inst, 'processingChildContext', true)

I think both. Here’s another example of adding a devtool for warnings: 251d6c3

onSetValueForProperty() will not be relevant here, it’s only called this way because it is called when React sets a value for DOM property.

In your case, you can add onBeginProcessingChildContext() and onEndProcessingChildContext(), and a new ReactInvalidSetStateWarningDevTool (or something like this) that would set the flag.

Also, since it’s related to React and not ReactDOM in particular, perhaps it makes sense to use ReactInstrumentation introduced in #6068 instead of ReactDOMInstrumentation.

This comment has been minimized.

Copy link
@raineroviir

raineroviir Feb 27, 2016

Author

Cool , Thanks!

So I notice I can't extend the internalInstance object to add the flag (that we are processingChildContext). What is a good object to add the flag to?

@@ -521,6 +522,7 @@ var ReactCompositeComponentMixin = {
}
return assign({}, currentContext, childContext);
}
ReactCurrentOwner.processingChildContext = null;

This comment has been minimized.

Copy link
@jimfb

jimfb Feb 25, 2016

Contributor

Bad to have an API that is null or true. If it is a boolean api, all values should be booleans.

This comment has been minimized.

Copy link
@raineroviir

raineroviir Feb 25, 2016

Author

Cool!Thanks for the notes I'll fix this

@facebook-github-bot

This comment has been minimized.

Copy link

commented Feb 27, 2016

@raineroviir updated the pull request.

@@ -48,6 +48,12 @@ var ReactDebugTool = {
}
}
},
onBeginProcessingChildContext(instance, boolean) {

This comment has been minimized.

Copy link
@jimfb

jimfb Feb 27, 2016

Contributor

boolean is a type. A good variable name would be something like isPprocessingChildContext. But why does this variable exist at all? Isn't it always true in this function and always false in the other?


var ReactInvalidSetStateWarningDevTool = {
onBeginProcessingChildContext(instance, boolean) {
warnInvalidSetState(instance, boolean)

This comment has been minimized.

Copy link
@jimfb

jimfb Feb 27, 2016

Contributor

Should set a scope-local variable on the devtool, not warn and/or set variables on the instance.

warnInvalidSetState(instance, boolean)
},
onEndProcessingChildContext(instance, boolean) {
warnInvalidSetState(instance, boolean)

This comment has been minimized.

Copy link
@jimfb

jimfb Feb 27, 2016

Contributor

Should set a scope-local variable on the devtool, not warn and/or set variables on the instance.

@@ -72,6 +72,12 @@ ReactComponent.prototype.setState = function(partialState, callback) {
'instead, use forceUpdate().'
);
}
if (__DEV__) {
warning(

This comment has been minimized.

Copy link
@jimfb

jimfb Feb 27, 2016

Contributor

Should emit a setState event, warn within the devtool.

This comment has been minimized.

Copy link
@raineroviir

raineroviir Feb 27, 2016

Author

Thank you for the notes!

@facebook-github-bot

This comment has been minimized.

Copy link

commented Feb 27, 2016

@raineroviir updated the pull request.

1 similar comment
@facebook-github-bot

This comment has been minimized.

Copy link

commented Feb 27, 2016

@raineroviir updated the pull request.

if (processingChildContext === false) {
return;
}
warning(

This comment has been minimized.

Copy link
@gaearon

gaearon Feb 28, 2016

Member

It might be more compact to write

warning(
  !processingChildContext,
  'setState(...): Cannot call setState inside getChildContext()'
);

This comment has been minimized.

Copy link
@raineroviir

raineroviir Feb 28, 2016

Author

You're right, Thanks

@gaearon

This comment has been minimized.

Copy link
Member

commented Feb 28, 2016

This looks great to me! Let’s hear what @jimfb has to say.

@raineroviir

This comment has been minimized.

Copy link
Author

commented Feb 28, 2016

I am failing a test locally not sure why it isn't failing in travis. Is that kinda worrying?

FAIL  src/renderers/shared/reconciler/__tests__/ReactCompositeComponent-test.js (3.028s)
● ReactCompositeComponent › it should pass context when re-rendered
  - Expected: 1 toBe: 0
        at Spec.eval (src/renderers/shared/reconciler/__tests__/ReactCompositeComponent-test.js:843:46)
@facebook-github-bot

This comment has been minimized.

Copy link

commented Feb 28, 2016

@raineroviir updated the pull request.

@@ -10,7 +10,7 @@
*/

'use strict';

var ReactInvalidSetStateWarningDevTool = require('./devtools/ReactInvalidSetStateWarningDevTool');

This comment has been minimized.

Copy link
@jimfb

jimfb Feb 29, 2016

Contributor

Normally we would just do require('ReactInvalidSetStateWarningDevTool'), so this seems weird. I'm assuming you did this because require('ReactInvalidSetStateWarningDevTool') didn't work for you because you didn't have an @providesModule at the top of that file.

If you add the standard copyright header to ReactInvalidSetStateWarningDevTool.js and specify the @providesModule, we should be able to fix this line.

This comment has been minimized.

Copy link
@raineroviir

raineroviir Feb 29, 2016

Author

Got it. That providesModule thing is super cool

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Feb 29, 2016

I looked into the failing unit test, it looks like a valid failure caused by this change (ie. needs to be fixed before we can merge). That test is not invoking setState() during getChildContext, but the warning appears to be firing.

I'm not sure why it didn't fail travis (cc @zpao).

@facebook-github-bot

This comment has been minimized.

Copy link

commented Feb 29, 2016

@raineroviir updated the pull request.

@jimfb jimfb referenced this pull request Feb 29, 2016

Closed

Jest failed, Travis Passed #6150

@facebook-github-bot

This comment has been minimized.

Copy link

commented Mar 1, 2016

@raineroviir updated the pull request.

@@ -783,7 +812,7 @@ describe('ReactCompositeComponent', function() {

render: function() {
var output = <Child />;
if (!this.state.flag) {
if (this.state.flag) {

This comment has been minimized.

Copy link
@jimfb

jimfb Mar 1, 2016

Contributor

What is going on here? Why this change?

This comment has been minimized.

Copy link
@raineroviir

raineroviir Mar 1, 2016

Author

I wanted to see why the test was throwing the warning

  1. in the original test we rendered <Parent> <Child /> <Parent>, and on setState we change the render to <Parent><span>Child</span></Parent>
  2. in the changed test which doesn't create a warning we go from <P><span>Child</span> </P> to <P><Child /><P>

I commited the change here for now in case it could benefit our search for the fix

This comment has been minimized.

Copy link
@jimfb

jimfb Mar 1, 2016

Contributor

Ok, well, we still need to fix so the original test passes.

This comment has been minimized.

Copy link
@raineroviir

raineroviir Mar 1, 2016

Author

Yeah! I will make the original test pass

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2016

@raineroviir

This comment has been minimized.

Copy link
Author

commented Mar 3, 2016

I've tried many things I'm not sure how to go about fixing the test.

@raineroviir raineroviir force-pushed the raineroviir:rainer branch from 1b8b939 to 754bbf9 Mar 3, 2016

@raineroviir

This comment has been minimized.

Copy link
Author

commented Mar 4, 2016

Wow I spent so long on this problem. I looked at some awesome code that vslinko wrote to fix this issue and it was so simple, I just did not fully understand the issue I was fixing. I made an early assumption of where the code should go and tried everything to make it work. Sigh. Like trying to stick a square peg in a round hole

@facebook-github-bot

This comment has been minimized.

Copy link

commented Mar 4, 2016

@raineroviir updated the pull request.

expect(instance.state.value).toBe(1);
expect(console.error.calls.length).toBe(1);
expect(console.error.argsForCall[0][0]).toBe(
'Warning: setState(...): Cannot call setState inside getChildContext()'

This comment has been minimized.

Copy link
@gaearon

gaearon Mar 5, 2016

Member

Nitpicking: let’s add () for consistency? As in Cannot call setState() inside getChildContext().

This comment has been minimized.

Copy link
@raineroviir
@gaearon

This comment has been minimized.

Copy link
Member

commented Mar 5, 2016

The earlier problem was likely due to return later in the method which caused onEnd* to be missing. You fixed it correctly by wrapping just the getChildContext() call.

Great job! Thank you for following through with this.

@gaearon

This comment has been minimized.

Copy link
Member

commented Mar 5, 2016

Would you mind squashing it as a single commit?

@raineroviir raineroviir force-pushed the raineroviir:rainer branch 2 times, most recently from 29f8ff4 to 3f2ffeb Mar 5, 2016

@facebook-github-bot

This comment has been minimized.

Copy link

commented Mar 5, 2016

@raineroviir updated the pull request.

@raineroviir raineroviir force-pushed the raineroviir:rainer branch from 3f2ffeb to 72e80df Mar 5, 2016

@facebook-github-bot

This comment has been minimized.

Copy link

commented Mar 5, 2016

@raineroviir updated the pull request.

@raineroviir raineroviir force-pushed the raineroviir:rainer branch from 72e80df to 9c1916d Mar 5, 2016

@facebook-github-bot

This comment has been minimized.

Copy link

commented Mar 5, 2016

@raineroviir updated the pull request.

@raineroviir

This comment has been minimized.

Copy link
Author

commented Mar 5, 2016

I totally just squashed all those commits! Thanks for all the great comments guys, no better way to learn how to do this stuff than by doing

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2016

This looks great to me, thanks @raineroviir!

jimfb added a commit that referenced this pull request Mar 5, 2016

Merge pull request #6121 from raineroviir/rainer
Fix #6114 - Calling setState inside getChildContext should warn

@jimfb jimfb merged commit 10f9476 into facebook:master Mar 5, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gaearon gaearon referenced this pull request Apr 28, 2016

Closed

Integrated warning on returning null #6636

0 of 2 tasks complete

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v0.14.9 #29

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.