[Fiber] ReactCompositeComponent-test.js#8950
Conversation
456abe5 to
d8cdefe
Compare
|
|
||
| var warnAboutUpdateInRender = function(instance : ReactClass<any>) { | ||
| warning( | ||
| ReactCurrentOwner.current == null, |
There was a problem hiding this comment.
This was always a bad pattern before because it assumed something about the "owner" pattern being a complete way to track this. It is also unnecessarily a deep dependency into an isomorphic library that can't be deduped. I believe that it is also not completely safe because ReactCurrentOwner.current doesn't get immediately reset to null like it does in Stack.
It would be better to track a DEV only specific flag that is designed for exactly this use case.
| exports.isProcessingChildContext = function() : boolean { | ||
| return _isProcessingChildContext; | ||
| }; | ||
| } |
There was a problem hiding this comment.
onBeginProcessingChildContext/onEndProcessingChildContext is over/early abstracted. Just set the flag to true and false like we do with ReactCurrentOwner. Seems like you can unify this with the render warning flag though.
| function scheduleTopLevelUpdate(current : Fiber, element : ReactNodeList, callback : ?Function) { | ||
| if (__DEV__) { | ||
| const owner = ReactCurrentOwner.current; | ||
| if (owner && typeof owner.tag === 'number') { |
There was a problem hiding this comment.
This extra tag check here is an indication that you're over using the ReactCurrentOwner concept. Should be fixed if you just do a render specific dev only flag for this specific use case.
76233b8 to
9558517
Compare
| type LifeCyclePhase = 'render' | 'getChildContext'; | ||
|
|
||
| const ReactDebugLifeCycle = { | ||
| current: (null : Fiber | null), |
There was a problem hiding this comment.
How is this different than ReactDebugCurrentFiber?
There was a problem hiding this comment.
Looks like it's a superset. I'll consolidate.
sebmarkbage
left a comment
There was a problem hiding this comment.
Waiting for consolidation.
Consolidated ReactDebugCurrentFiber and ReactDebugLifeCycle
| } | ||
|
|
||
| if (__DEV__) { | ||
| if (fiber.tag === ClassComponent) { |
There was a problem hiding this comment.
I guess this could be relevant to top level ReactDOM.render too?
There was a problem hiding this comment.
I guess you have a separate one for that. Makes sense.
Stack uses a module called ReactInvalidSetStateWarningHook, but all it does is warn about setState inside getChildContext. Doesn't seem worth keeping around, so I didn't use it for Fiber, but if I'm mistaken I'll change it.
Use this to detect setState inside of render instead of relying on the owner.
6cf3863 to
b21c2a5
Compare
b21c2a5 to
393cdbc
Compare
Fixes tests in ReactCompositeComponent-test.js
Overlaps with #8949 so I based it on that. Review that one first.