[Fiber] Add top level render callbacks into ReactDOMFiber and ReactNoop #8102
Conversation
koba04
force-pushed
the
add-top-level-render-callbacks
branch
from
b7d470f
to
465047a
Compare
| @@ -43,6 +43,26 @@ describe('ReactDOMFiber', () => { | |||
| expect(container.textContent).toEqual('10'); | |||
| }); | |||
|
|
|||
| it('should be called a callback argument', () => { | |||
There was a problem hiding this comment.
I noticed the stack reconciler also passed the public instance as this into the callback.
I found it weird but I guess we should do it here as well for feature parity.
See the test verifying it (it will fail due to missing unstable_batchedUpdates but you can temporarily comment it to check if a future fix works).
There was a problem hiding this comment.
@gaearon Thanks! I've fixed that this in render callbacks is the public instance.
Then I confirmed the tests linked from your comment are passed when unstable_batchedUpdates is mocked.
I'll fix it after this PR was merged.
| ReactNoop.flush(); | ||
|
|
||
| // TODO: Test bail out of host components. This is currently unobservable. | ||
|
|
||
| // Since this is an update, it should bail out and reuse the work from | ||
| // Header and Content. | ||
| expect(ops).toEqual(['Foo', 'Content']); | ||
| expect(ops).toEqual(['Foo', 'Content', 'renderCallbackCalled']); | ||
|
|
||
| }); |
There was a problem hiding this comment.
Should we also have a test verifying that if you queue two callbacks without flushing, and then flush once, both get called?
There was a problem hiding this comment.
In this case, I think two callbacks should be called after flushing. I've added a test for it.
| case HostContainer: { | ||
| const instance = finishedWork.stateNode; | ||
| if (instance.callbackList) { | ||
| const { callbackList } = instance; |
There was a problem hiding this comment.
Style nit:
const {callbackList} = instance;There was a problem hiding this comment.
@gaearon spaces inside curly braces aren't needed?
I can see spaces inside curly braces in the ReactFiber codebase.
Is there a rule for that?
|
This looks good to me but I don't have enough context about how Fiber is supposed to work yet so let's wait for @sebmarkbage or @acdlite. |
| // if the root is a HostContainer, it may have a callback. | ||
| if (finishedWork.tag === HostContainer) { | ||
| const current = finishedWork.alternate; | ||
| commitLifeCycles(current, finishedWork); |
There was a problem hiding this comment.
This needs to move in behind the effectTag check. Because we don't want to invoke this unless the root updates. This can happen if a render is scheduled, but we do a deep setState update at a higher priority first.
This isn't actually possible right now without #7457 but it will be.
There was a problem hiding this comment.
I've moved it to behind the effectTag check.
| @@ -165,6 +165,11 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) { | |||
| const current = finishedWork.alternate; | |||
| commitWork(current, finishedWork); | |||
| } | |||
| // if the root is a HostContainer, it may have a callback. | |||
| if (finishedWork.tag === HostContainer) { | |||
There was a problem hiding this comment.
This tag will be checked inside of commitLifeCycles so there is no need to do it here. You can just remove the conditional.
|
Thanks a lot! This should bump our passing unit tests a bit! |
|
@sebmarkbage @gaearon Thank you for your help!!! |
…op (facebook#8102) * [Fiber] Add top level render callbacks into ReactDOMFiber and ReactNoop * [Fiber] Support multiple render callbacks * [Fiber] `this` in render callbacks are public instances * [Fiber] commitLifeCycles move to behind the effectTag check
This PR is to fix an issue listed in #7925 (Phase 5: Low pri).
In this PR, render callbacks are called in the
commitLifeCycles, which is called from at the end ofcommitAllWorkif a target fiber isHostContainer.