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] String refs and owner tracking #8099

Merged
merged 5 commits into from
Oct 26, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/isomorphic/classic/element/ReactCurrentOwner.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
'use strict';

import type { ReactInstance } from 'ReactInstanceType';
import type { Fiber } from 'ReactFiber';

/**
* Keeps track of the current owner.
Expand All @@ -26,7 +27,7 @@ var ReactCurrentOwner = {
* @internal
* @type {ReactComponent}
*/
current: (null: null | ReactInstance),
current: (null: null | ReactInstance | Fiber),

};

Expand Down
7 changes: 4 additions & 3 deletions src/isomorphic/hooks/ReactComponentTreeHook.js
Original file line number Diff line number Diff line change
Expand Up @@ -315,9 +315,10 @@ var ReactComponentTreeHook = {
}

var currentOwner = ReactCurrentOwner.current;
var id = currentOwner && currentOwner._debugID;

info += ReactComponentTreeHook.getStackAddendumByID(id);
if (currentOwner && typeof currentOwner._debugID === 'number') {
var id = currentOwner && currentOwner._debugID;
info += ReactComponentTreeHook.getStackAddendumByID(id);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm actually about to try to make this file work with Fiber but this is fine too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah tbh I barely looked at what this file does; just trying to get it merged for now :D

return info;
},

Expand Down
6 changes: 5 additions & 1 deletion src/renderers/native/findNodeHandle.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ var ReactInstanceMap = require('ReactInstanceMap');
var invariant = require('invariant');
var warning = require('warning');

import type { ReactInstance } from 'ReactInstanceType';

/**
* ReactNative vs ReactWeb
* -----------------------
Expand Down Expand Up @@ -50,7 +52,8 @@ var warning = require('warning');

function findNodeHandle(componentOrHandle: any): ?number {
if (__DEV__) {
var owner = ReactCurrentOwner.current;
// TODO: fix this unsafe cast to work with Fiber.
var owner = ((ReactCurrentOwner.current: any): ReactInstance | null);
if (owner !== null) {
warning(
owner._warnedAboutRefsInRender,
Expand All @@ -61,6 +64,7 @@ function findNodeHandle(componentOrHandle: any): ?number {
'componentDidUpdate instead.',
owner.getName() || 'A component'
);

owner._warnedAboutRefsInRender = true;
}
}
Expand Down
37 changes: 32 additions & 5 deletions src/renderers/shared/fiber/ReactChildFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ var ReactReifiedYield = require('ReactReifiedYield');
var ReactTypeOfSideEffect = require('ReactTypeOfSideEffect');
var ReactTypeOfWork = require('ReactTypeOfWork');

var emptyObject = require('emptyObject');
var getIteratorFn = require('getIteratorFn');

const {
Expand All @@ -47,6 +48,7 @@ const {
const isArray = Array.isArray;

const {
ClassComponent,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like ClassComponent isn't used anywhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I imported it for this purpose but forgot: https://github.com/facebook/react/pull/8099/files#r85031873

:D

HostText,
CoroutineComponent,
YieldComponent,
Expand All @@ -62,6 +64,31 @@ const {
Deletion,
} = ReactTypeOfSideEffect;

function transferRef(current: ?Fiber, workInProgress: Fiber, element: ReactElement<any>) {
if (typeof element.ref === 'string') {
if (element._owner) {
const ownerFiber : ?Fiber = (element._owner : any);
if (ownerFiber && ownerFiber.tag === ClassComponent) {
const stringRef = element.ref;
// Check if previous string ref matches new string ref
if (current && current.ref && current.ref._stringRef === stringRef) {
workInProgress.ref = current.ref;
return;
}
const inst = ownerFiber.stateNode;
const ref = function(value) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function won't get a name. We should make any functions calling user code named for better stack traces.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Never mind, I misunderstood. This doesn't call into user code.

const refs = inst.refs === emptyObject ? (inst.refs = {}) : inst.refs;
refs[stringRef] = value;
};
ref._stringRef = stringRef;
workInProgress.ref = ref;
}
}
} else {
workInProgress.ref = element.ref;
}
}

// This wrapper function exists because I expect to clone the code in each path
// to be able to optimize each path individually by branching early. This needs
// a compiler or we can do it manually. Helpers that don't need this branching
Expand Down Expand Up @@ -221,13 +248,13 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
if (current == null || current.type !== element.type) {
// Insert
const created = createFiberFromElement(element, priority);
created.ref = element.ref;
transferRef(current, created, element);
created.return = returnFiber;
return created;
} else {
// Move based on index
const existing = useFiber(current, priority);
existing.ref = element.ref;
transferRef(current, existing, element);
existing.pendingProps = element.props;
existing.return = returnFiber;
return existing;
Expand Down Expand Up @@ -319,7 +346,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
switch (newChild.$$typeof) {
case REACT_ELEMENT_TYPE: {
const created = createFiberFromElement(newChild, priority);
created.ref = newChild.ref;
transferRef(null, created, newChild);
created.return = returnFiber;
return created;
}
Expand Down Expand Up @@ -653,7 +680,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
if (child.type === element.type) {
deleteRemainingChildren(returnFiber, child.sibling);
const existing = useFiber(child, priority);
existing.ref = element.ref;
transferRef(child, existing, element);
existing.pendingProps = element.props;
existing.return = returnFiber;
return existing;
Expand All @@ -668,7 +695,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
}

const created = createFiberFromElement(element, priority);
created.ref = element.ref;
transferRef(currentFirstChild, created, element);
created.return = returnFiber;
return created;
}
Expand Down
26 changes: 14 additions & 12 deletions src/renderers/shared/fiber/ReactFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,18 @@ var {
NoEffect,
} = require('ReactTypeOfSideEffect');

// An Instance is shared between all versions of a component. We can easily
// break this out into a separate object to avoid copying so much to the
// alternate versions of the tree. We put this on a single object for now to
// minimize the number of objects created during the initial render.
type Instance = {
// A Fiber is work on a Component that needs to be done or was done. There can
// be more than one per component.
export type Fiber = {
// These first fields are conceptually members of an Instance. This used to
// be split into a separate type and intersected with the other Fiber fields,
// but until Flow fixes its intersection bugs, we've merged them into a
// single type.

// An Instance is shared between all versions of a component. We can easily
// break this out into a separate object to avoid copying so much to the
// alternate versions of the tree. We put this on a single object for now to
// minimize the number of objects created during the initial render.

// Tag identifying the type of fiber.
tag: TypeOfWork,
Expand All @@ -61,11 +68,7 @@ type Instance = {
// parent : Instance -> return The parent happens to be the same as the
// return fiber since we've merged the fiber and instance.

};

// A Fiber is work on a Component that needs to be done or was done. There can
// be more than one per component.
export type Fiber = Instance & {
// Remaining fields belong to Fiber

// The Fiber to return to after finishing processing this one.
// This is effectively the parent, but there can be multiple parents (two)
Expand All @@ -80,7 +83,7 @@ export type Fiber = Instance & {

// The ref last used to attach this node.
// I'll avoid adding an owner field for prod and model that as functions.
ref: null | (handle : ?Object) => void,
ref: null | (((handle : ?Object) => void) & { _stringRef: ?string }),

// Input is the data coming into process this fiber. Arguments. Props.
pendingProps: any, // This type will be more specific once we overload the tag.
Expand Down Expand Up @@ -327,4 +330,3 @@ exports.createFiberFromYield = function(yieldNode : ReactYield, priorityLevel :
fiber.pendingProps = {};
return fiber;
};

19 changes: 17 additions & 2 deletions src/renderers/shared/fiber/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import type { ReactCoroutine } from 'ReactCoroutine';
import type { Fiber } from 'ReactFiber';
import type { HostConfig } from 'ReactFiberReconciler';
import type { PriorityLevel } from 'ReactPriorityLevel';
import ReactCurrentOwner from 'ReactCurrentOwner';

var {
mountChildFibersInPlace,
Expand Down Expand Up @@ -151,7 +152,12 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>, s
}
}

var nextChildren = fn(props);
if (__DEV__) {
ReactCurrentOwner.current = workInProgress;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we decided that we didn't need this for functional components? EDIT: Oh, it is DEV only. nvm.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought what we decided was we didn't need it when constructing a class component. Still need it for rendering a functional component. Might be best to remove this until it's actually needed regardless.

var nextChildren = fn(props);
} else {
var nextChildren = fn(props);
}
reconcileChildren(current, workInProgress, nextChildren);
return workInProgress.child;
}
Expand All @@ -176,6 +182,7 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>, s
}
// Rerender
const instance = workInProgress.stateNode;
ReactCurrentOwner.current = workInProgress;
const nextChildren = instance.render();
reconcileChildren(current, workInProgress, nextChildren);
return workInProgress.child;
Expand Down Expand Up @@ -237,12 +244,20 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>, s
}
var fn = workInProgress.type;
var props = workInProgress.pendingProps;
var value = fn(props);

if (__DEV__) {
ReactCurrentOwner.current = workInProgress;
var value = fn(props);
} else {
var value = fn(props);
}

if (typeof value === 'object' && value && typeof value.render === 'function') {
// Proceed under the assumption that this is a class instance
workInProgress.tag = ClassComponent;
adoptClassInstance(workInProgress, value);
mountClassInstance(workInProgress);
ReactCurrentOwner.current = workInProgress;
value = value.render();
} else {
// Proceed under the assumption that this is a functional component
Expand Down
3 changes: 3 additions & 0 deletions src/renderers/shared/fiber/ReactFiberScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import type { PriorityLevel } from 'ReactPriorityLevel';
var ReactFiberBeginWork = require('ReactFiberBeginWork');
var ReactFiberCompleteWork = require('ReactFiberCompleteWork');
var ReactFiberCommitWork = require('ReactFiberCommitWork');
var ReactCurrentOwner = require('ReactCurrentOwner');

var { cloneFiber } = require('ReactFiber');

Expand Down Expand Up @@ -298,6 +299,8 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
}
}

ReactCurrentOwner.current = null;

return next;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1072,4 +1072,28 @@ describe('ReactIncrementalSideEffects', () => {
// TODO: Test that mounts, updates, refs, unmounts and deletions happen in the
// expected way for aborted and resumed render life-cycles.

it('supports string refs', () => {
var fooInstance = null;

class Bar extends React.Component {
componentDidMount() {
this.test = 'test';
}
render() {
return <div />;
}
}

class Foo extends React.Component {
render() {
fooInstance = this;
return <Bar ref="bar" />;
}
}

ReactNoop.render(<Foo />);
ReactNoop.flush();

expect(fooInstance.refs.bar.test).toEqual('test');
});
});