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

[Fiber] Add top level render callbacks into ReactDOMFiber and ReactNoop #8102

Merged
merged 4 commits into from
Oct 26, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[Fiber] Add top level render callbacks into ReactDOMFiber and ReactNoop
  • Loading branch information
koba04 committed Oct 26, 2016
commit 465047a1db7906db0ef2fb028bf9f589b4c760e5
6 changes: 3 additions & 3 deletions src/renderers/dom/fiber/ReactDOMFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -124,13 +124,13 @@ function warnAboutUnstableUse() {

var ReactDOM = {

render(element : ReactElement<any>, container : DOMContainerElement) {
render(element : ReactElement<any>, container : DOMContainerElement, callback: ?Function) {
warnAboutUnstableUse();
let root;
if (!container._reactRootContainer) {
root = container._reactRootContainer = DOMRenderer.mountContainer(element, container);
root = container._reactRootContainer = DOMRenderer.mountContainer(element, container, callback);
} else {
DOMRenderer.updateContainer(element, root = container._reactRootContainer);
DOMRenderer.updateContainer(element, root = container._reactRootContainer, callback);
}
return DOMRenderer.getPublicRootInstance(root);
},
Expand Down
20 changes: 20 additions & 0 deletions src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,26 @@ describe('ReactDOMFiber', () => {
expect(container.textContent).toEqual('10');
});

it('should be called a callback argument', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

// mounting phase
let called = false;
ReactDOM.render(
<div>Foo</div>,
container,
() => called = true
);
expect(called).toEqual(true);

// updating phase
called = false;
ReactDOM.render(
<div>Foo</div>,
container,
() => called = true
);
expect(called).toEqual(true);
});

if (ReactDOMFeatureFlags.useFiber) {
it('should render a component returning strings directly from render', () => {
const Text = ({value}) => value;
Expand Down
6 changes: 3 additions & 3 deletions src/renderers/noop/ReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -146,11 +146,11 @@ var ReactNoop = {

root: rootContainer,

render(element : ReactElement<any>) {
render(element : ReactElement<any>, callback: ?Function) {
if (!root) {
root = NoopRenderer.mountContainer(element, rootContainer);
root = NoopRenderer.mountContainer(element, rootContainer, callback);
} else {
NoopRenderer.updateContainer(element, root);
NoopRenderer.updateContainer(element, root, callback);
}
},

Expand Down
8 changes: 8 additions & 0 deletions src/renderers/shared/fiber/ReactFiberCommitWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,14 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
attachRef(current, finishedWork, instance);
return;
}
case HostContainer: {
const instance = finishedWork.stateNode;
if (instance.callbackList) {
const { callbackList } = instance;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style nit:

const {callbackList} = instance;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

instance.callbackList = null;
callCallbacks(callbackList, instance);
}
}
case HostComponent: {
const instance : I = finishedWork.stateNode;
attachRef(current, finishedWork, instance);
Expand Down
16 changes: 14 additions & 2 deletions src/renderers/shared/fiber/ReactFiberReconciler.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import type { PriorityLevel } from 'ReactPriorityLevel';
var { createFiberRoot } = require('ReactFiberRoot');
var ReactFiberScheduler = require('ReactFiberScheduler');

var { createUpdateQueue, addCallbackToQueue } = require('ReactFiberUpdateQueue');

if (__DEV__) {
var ReactFiberInstrumentation = require('ReactFiberInstrumentation');
}
Expand Down Expand Up @@ -74,9 +76,14 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) :

return {

mountContainer(element : ReactElement<any>, containerInfo : C) : OpaqueNode {
mountContainer(element : ReactElement<any>, containerInfo : C, callback: ?Function) : OpaqueNode {
const root = createFiberRoot(containerInfo);
const container = root.current;
if (callback) {
const queue = createUpdateQueue(null);
addCallbackToQueue(queue, callback);
root.callbackList = queue;
}
// TODO: Use pending work/state instead of props.
// TODO: This should not override the pendingWorkPriority if there is
// higher priority work in the subtree.
Expand All @@ -94,9 +101,14 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) :
return container;
},

updateContainer(element : ReactElement<any>, container : OpaqueNode) : void {
updateContainer(element : ReactElement<any>, container : OpaqueNode, callback: ?Function) : void {
// TODO: If this is a nested container, this won't be the root.
const root : FiberRoot = (container.stateNode : any);
if (callback) {
const queue = createUpdateQueue(null);
addCallbackToQueue(queue, callback);
root.callbackList = queue;
}
// TODO: Use pending work/state instead of props.
root.current.pendingProps = element;

Expand Down
4 changes: 4 additions & 0 deletions src/renderers/shared/fiber/ReactFiberRoot.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
'use strict';

import type { Fiber } from 'ReactFiber';
import type { UpdateQueue } from 'ReactFiberUpdateQueue';

const { createHostContainerFiber } = require('ReactFiber');

Expand All @@ -25,6 +26,8 @@ export type FiberRoot = {
isScheduled: boolean,
// The work schedule is a linked list.
nextScheduledRoot: ?FiberRoot,
// Linked list of callbacks to call after updates are committed.
callbackList: ?UpdateQueue,
};

exports.createFiberRoot = function(containerInfo : any) : FiberRoot {
Expand All @@ -36,6 +39,7 @@ exports.createFiberRoot = function(containerInfo : any) : FiberRoot {
containerInfo: containerInfo,
isScheduled: false,
nextScheduledRoot: null,
callbackList: null,
};
uninitializedFiber.stateNode = root;
return root;
Expand Down
5 changes: 5 additions & 0 deletions src/renderers/shared/fiber/ReactFiberScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tag will be checked inside of commitLifeCycles so there is no need to do it here. You can just remove the conditional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I've fixed it!

const current = finishedWork.alternate;
commitLifeCycles(current, finishedWork);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved it to behind the effectTag check.

}
}

function resetWorkPriority(workInProgress : Fiber) {
Expand Down
14 changes: 9 additions & 5 deletions src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ describe('ReactIncremental', () => {

it('should render a simple component, in steps if needed', () => {

var renderCallbackCalled = false;
var barCalled = false;
function Bar() {
barCalled = true;
Expand All @@ -52,17 +53,20 @@ describe('ReactIncremental', () => {
];
}

ReactNoop.render(<Foo />);
ReactNoop.render(<Foo />, () => renderCallbackCalled = true);
expect(fooCalled).toBe(false);
expect(barCalled).toBe(false);
expect(renderCallbackCalled).toBe(false);
// Do one step of work.
ReactNoop.flushDeferredPri(7 + 5);
expect(fooCalled).toBe(true);
expect(barCalled).toBe(false);
expect(renderCallbackCalled).toBe(false);
// Do the rest of the work.
ReactNoop.flushDeferredPri(50);
expect(fooCalled).toBe(true);
expect(barCalled).toBe(true);
expect(renderCallbackCalled).toBe(true);
});

it('updates a previous render', () => {
Expand Down Expand Up @@ -98,21 +102,21 @@ describe('ReactIncremental', () => {
);
}

ReactNoop.render(<Foo text="foo" />);
ReactNoop.render(<Foo text="foo" />, () => ops.push('renderCallbackCalled'));
ReactNoop.flush();

expect(ops).toEqual(['Foo', 'Header', 'Content', 'Footer']);
expect(ops).toEqual(['Foo', 'Header', 'Content', 'Footer', 'renderCallbackCalled']);

ops = [];

ReactNoop.render(<Foo text="bar" />);
ReactNoop.render(<Foo text="bar" />, () => ops.push('renderCallbackCalled'));
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']);

});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also have a test verifying that if you queue two callbacks without flushing, and then flush once, both get called?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, I think two callbacks should be called after flushing. I've added a test for it.


Expand Down