[Fiber] ReactCompositeComponent-test.js #8950
Conversation
acdlite
force-pushed
the
fibercompositecomponenttests
branch
10 times, most recently
from
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.
acdlite
force-pushed
the
fibercompositecomponenttests
branch
2 times, most recently
from
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
sebmarkbage
left a comment
There was a problem hiding this comment.
Waiting for consolidation.
acdlite
dismissed
sebmarkbage’s stale review
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.
acdlite
force-pushed
the
fibercompositecomponenttests
branch
from
6cf3863 to
b21c2a5
Compare
acdlite
force-pushed
the
fibercompositecomponenttests
branch
from
b21c2a5 to
393cdbc
Compare
3 participants
Fixes tests in ReactCompositeComponent-test.js
Overlaps with #8949 so I based it on that. Review that one first.