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 ReactFiberReconciler annotation to include PI #8751

Merged
merged 9 commits into from
May 30, 2017
2 changes: 2 additions & 0 deletions scripts/fiber/tests-passing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,7 @@ src/renderers/__tests__/refs-test.js
* coerces numbers to strings
* attaches, detaches from fiber component with stack layer
* attaches, detaches from stack component with fiber layer
* attaches and detaches root refs

src/renderers/art/__tests__/ReactART-test.js
* should have the correct lifecycle state
Expand Down Expand Up @@ -1972,6 +1973,7 @@ src/renderers/testing/__tests__/ReactTestRenderer-test.js
* can update text nodes
* toTree() renders simple components returning host components
* toTree() handles null rendering components
* root instance and createNodeMock ref return the same value
* toTree() renders complicated trees of composites and hosts
* can update text nodes when rendered as root
* can render and update root fragments
Expand Down
84 changes: 84 additions & 0 deletions src/renderers/__tests__/refs-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -409,3 +409,87 @@ describe('string refs between fiber and stack', () => {
}
});
});

describe('root level refs', () => {
beforeEach(() => {
var ReactFeatureFlags = require('ReactFeatureFlags');
ReactFeatureFlags.disableNewFiberFeatures = false;
});

it('attaches and detaches root refs', () => {
var ReactDOM = require('react-dom');
var inst = null;

// host node
var ref = jest.fn(value => (inst = value));
var container = document.createElement('div');
var result = ReactDOM.render(<div ref={ref} />, container);
expect(ref).toHaveBeenCalledTimes(1);
expect(ref.mock.calls[0][0]).toBeInstanceOf(HTMLDivElement);
expect(result).toBe(ref.mock.calls[0][0]);
ReactDOM.unmountComponentAtNode(container);
expect(ref).toHaveBeenCalledTimes(2);
expect(ref.mock.calls[1][0]).toBe(null);

// composite
class Comp extends React.Component {
method() {
return true;
}
render() {
return <div>Comp</div>;
}
}

inst = null;
ref = jest.fn(value => (inst = value));
result = ReactDOM.render(<Comp ref={ref} />, container);

expect(ref).toHaveBeenCalledTimes(1);
expect(inst).toBeInstanceOf(Comp);
expect(result).toBe(inst);

// ensure we have the correct instance
expect(result.method()).toBe(true);
expect(inst.method()).toBe(true);

ReactDOM.unmountComponentAtNode(container);
expect(ref).toHaveBeenCalledTimes(2);
expect(ref.mock.calls[1][0]).toBe(null);

if (ReactDOMFeatureFlags.useFiber) {
// fragment
inst = null;
ref = jest.fn(value => (inst = value));
var divInst = null;
var ref2 = jest.fn(value => (divInst = value));
result = ReactDOM.render(
[<Comp ref={ref} key="a" />, 5, <div ref={ref2} key="b">Hello</div>],
container,
);

// first call should be `Comp`
expect(ref).toHaveBeenCalledTimes(1);
expect(ref.mock.calls[0][0]).toBeInstanceOf(Comp);
expect(result).toBe(ref.mock.calls[0][0]);

expect(ref2).toHaveBeenCalledTimes(1);
expect(divInst).toBeInstanceOf(HTMLDivElement);
expect(result).not.toBe(divInst);

ReactDOM.unmountComponentAtNode(container);
expect(ref).toHaveBeenCalledTimes(2);
expect(ref.mock.calls[1][0]).toBe(null);
expect(ref2).toHaveBeenCalledTimes(2);
expect(ref2.mock.calls[1][0]).toBe(null);

// null
result = ReactDOM.render(null, container);
expect(result).toBe(null);

// primitives
result = ReactDOM.render(5, container);
expect(result).toBeInstanceOf(Text);
}
});
});
10 changes: 8 additions & 2 deletions src/renderers/shared/fiber/ReactFiberCommitWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -518,8 +518,14 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
function commitAttachRef(finishedWork: Fiber) {
const ref = finishedWork.ref;
if (ref !== null) {
const instance = getPublicInstance(finishedWork.stateNode);
ref(instance);
const instance = finishedWork.stateNode;
switch (finishedWork.tag) {
case HostComponent:
ref(getPublicInstance(instance));
break;
default:
ref(instance);
}
}
}

Expand Down
14 changes: 11 additions & 3 deletions src/renderers/shared/fiber/ReactFiberReconciler.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ var {
} = require('ReactFiberContext');
var {createFiberRoot} = require('ReactFiberRoot');
var ReactFiberScheduler = require('ReactFiberScheduler');
var {HostComponent} = require('ReactTypeOfWork');

if (__DEV__) {
var warning = require('fbjs/lib/warning');
Expand Down Expand Up @@ -167,6 +168,8 @@ getContextForSubtree._injectFiber(function(fiber: Fiber) {
module.exports = function<T, P, I, TI, PI, C, CX, PL>(
config: HostConfig<T, P, I, TI, PI, C, CX, PL>,
): Reconciler<C, I, TI> {
var {getPublicInstance} = config;

var {
scheduleUpdate,
getPriorityContext,
Expand Down Expand Up @@ -269,15 +272,20 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(

getPublicRootInstance(
container: OpaqueRoot,
): ReactComponent<any, any, any> | I | TI | null {
): ReactComponent<any, any, any> | PI | null {
const containerFiber = container.current;
if (!containerFiber.child) {
return null;
}
return containerFiber.child.stateNode;
switch (containerFiber.child.tag) {
case HostComponent:
return getPublicInstance(containerFiber.child.stateNode);
default:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This case should match on ClassComponent only. It won't type check if we use proper disjoint unions on Fibers.

That type error would be correct since this can be other types such as fragments, coroutines etc. Those should do something reasonable like throw, drill down (like findDOMNode does) or return null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It won't type check if we use proper disjoint unions on Fibers.

Is that PR any closer to landing by chance?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It'll have to be a massive rebase. We could try it again though. There are some new features in Flow that can make that palatable now.

return containerFiber.child.stateNode;
}
},

findHostInstance(fiber: Fiber): I | TI | null {
findHostInstance(fiber: Fiber): PI | null {
const hostFiber = findCurrentHostFiber(fiber);
if (hostFiber === null) {
return null;
Expand Down
2 changes: 1 addition & 1 deletion src/renderers/testing/ReactTestRendererFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ var TestRenderer = ReactFiberReconciler({

useSyncScheduling: true,

getPublicInstance(inst) {
getPublicInstance(inst: Instance | TextInstance): * {
switch (inst.tag) {
case 'INSTANCE':
const createNodeMock = inst.rootContainerInstance.createNodeMock;
Expand Down
11 changes: 11 additions & 0 deletions src/renderers/testing/__tests__/ReactTestRenderer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -568,6 +568,17 @@ describe('ReactTestRenderer', () => {
});
});

it('root instance and createNodeMock ref return the same value', () => {
var createNodeMock = ref => ({node: ref});
var refInst = null;
var renderer = ReactTestRenderer.create(
<div ref={ref => (refInst = ref)} />,
{createNodeMock},
);
var root = renderer.getInstance();
expect(root).toEqual(refInst);
});

it('toTree() renders complicated trees of composites and hosts', () => {
// SFC returning host. no children props.
var Qoo = () => <span className="Qoo">Hello World!</span>;
Expand Down