Skip to content

[Fiber] Update root children using appendChild/insertBefore/removeChild #8400

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.

Sign up for GitHub

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

Merged
sebmarkbage merged 4 commits into from
Nov 24, 2016
Merged

[Fiber] Update root children using appendChild/insertBefore/removeChild #8400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
78add2d
Update root children using the appendChild/insertBefore/removeChild m…
sebmarkbage Nov 23, 2016
fde18a4
Assert that we always find a parent for DOM mutations
sebmarkbage Nov 23, 2016
024e2a0
Remove recursion from unmounting portals (#1)
gaearon Nov 24, 2016
ea34204
Split initial children out of createInstance
sebmarkbage Nov 24, 2016
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
1 change: 0 additions & 1 deletion scripts/fiber/tests-failing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,4 +120,3 @@ src/renderers/shared/stack/reconciler/__tests__/refs-test.js

src/test/__tests__/ReactTestUtils-test.js
* traverses children in the correct order
* should support injected wrapper components as DOM components
1 change: 1 addition & 0 deletions scripts/fiber/tests-passing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1560,6 +1560,7 @@ src/test/__tests__/ReactTestUtils-test.js
* can scryRenderedDOMComponentsWithClass with TextComponent
* can scryRenderedDOMComponentsWithClass with className contains \n
* can scryRenderedDOMComponentsWithClass with multiple classes
* should support injected wrapper components as DOM components
* should change the value of an input field
* should change the value of an input field in a component
* should throw when attempting to use ReactTestUtils.Simulate with shallow rendering
Expand Down
61 changes: 26 additions & 35 deletions src/renderers/dom/fiber/ReactDOMFiber.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
'use strict';

import type { Fiber } from 'ReactFiber';
import type { HostChildren } from 'ReactFiberReconciler';
import type { ReactNodeList } from 'ReactTypes';

var ReactBrowserEventEmitter = require('ReactBrowserEventEmitter');
Expand All @@ -38,6 +37,8 @@ var {
} = ReactDOMFiberComponent;
var { precacheFiberNode } = ReactDOMComponentTree;

const DOCUMENT_NODE = 9;

ReactDOMInjection.inject();
ReactControlledComponent.injection.injectFiberControlledHostComponent(
ReactDOMFiberComponent
Expand All @@ -53,23 +54,6 @@ type Props = { className ?: string };
type Instance = Element;
type TextInstance = Text;

function recursivelyAppendChildren(parent : Element, child : HostChildren<Instance | TextInstance>) {
if (!child) {
return;
}
/* $FlowFixMe: Element and Text should have this property. */
if (child.nodeType === 1 || child.nodeType === 3) {
/* $FlowFixMe: Refinement issue. I don't know how to express different. */
parent.appendChild(child);
} else {
/* As a result of the refinement issue this type isn't known. */
let node : any = child;
do {
recursivelyAppendChildren(parent, node.output);
} while (node = node.sibling);
}
}

let eventsEnabled : ?boolean = null;
let selectionInformation : ?mixed = null;

Expand All @@ -88,27 +72,28 @@ var DOMRenderer = ReactFiberReconciler({
eventsEnabled = null;
},

updateContainer(container : Container, children : HostChildren<Instance | TextInstance>) : void {
// TODO: Containers should update similarly to other parents.
container.innerHTML = '';
recursivelyAppendChildren(container, children);
},

createInstance(
type : string,
props : Props,
children : HostChildren<Instance | TextInstance>,
internalInstanceHandle : Object
) : Instance {
const root = document.body; // HACK
const root = document.documentElement; // HACK

const domElement : Instance = createElement(type, props, root);
precacheFiberNode(internalInstanceHandle, domElement);
recursivelyAppendChildren(domElement, children);
setInitialProperties(domElement, type, props, root);
return domElement;
},

appendInitialChild(parentInstance : Instance, child : Instance | TextInstance) : void {
Copy link
Contributor

@edvinerikson edvinerikson Nov 24, 2016

Choose a reason for hiding this comment

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

will this only be called on the initial mount? or also when new children get's appended on a already mounted node?

will appendChild or appendInitialChild handle: ["Foo"] -> ["Foo", "Bar"] ?

Copy link
Collaborator

@gaearon gaearon Nov 24, 2016

Choose a reason for hiding this comment

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

This would be handled by appendChild if the parent is already in the DOM.

parentInstance.appendChild(child);
},

finalizeInitialChildren(domElement : Instance, type : string, props : Props) : void {
const root = document.documentElement; // HACK

setInitialProperties(domElement, type, props, root);
},

prepareUpdate(
domElement : Instance,
oldProps : Props,
Expand All @@ -124,7 +109,7 @@ var DOMRenderer = ReactFiberReconciler({
internalInstanceHandle : Object
) : void {
var type = domElement.tagName.toLowerCase(); // HACK
var root = document.body; // HACK
var root = document.documentElement; // HACK
// Update the internal instance handle so that we know which props are
// the current ones.
precacheFiberNode(internalInstanceHandle, domElement);
Expand All @@ -141,19 +126,19 @@ var DOMRenderer = ReactFiberReconciler({
textInstance.nodeValue = newText;
},

appendChild(parentInstance : Instance, child : Instance | TextInstance) : void {
appendChild(parentInstance : Instance | Container, child : Instance | TextInstance) : void {
parentInstance.appendChild(child);
},

insertBefore(
parentInstance : Instance,
parentInstance : Instance | Container,
child : Instance | TextInstance,
beforeChild : Instance | TextInstance
) : void {
parentInstance.insertBefore(child, beforeChild);
},

removeChild(parentInstance : Instance, child : Instance | TextInstance) : void {
removeChild(parentInstance : Instance | Container, child : Instance | TextInstance) : void {
parentInstance.removeChild(child);
},

Expand All @@ -177,9 +162,15 @@ function warnAboutUnstableUse() {
warned = true;
}

function renderSubtreeIntoContainer(parentComponent : ?ReactComponent<any, any, any>, element : ReactElement<any>, container : DOMContainerElement, callback: ?Function) {
function renderSubtreeIntoContainer(parentComponent : ?ReactComponent<any, any, any>, element : ReactElement<any>, containerNode : DOMContainerElement | Document, callback: ?Function) {
let container : DOMContainerElement =
containerNode.nodeType === DOCUMENT_NODE ? (containerNode : any).documentElement : (containerNode : any);
let root;
if (!container._reactRootContainer) {
// First clear any existing content.
while (container.lastChild) {
container.removeChild(container.lastChild);
}
root = container._reactRootContainer = DOMRenderer.mountContainer(element, container, parentComponent, callback);
} else {
DOMRenderer.updateContainer(element, root = container._reactRootContainer, parentComponent, callback);
Expand All @@ -194,12 +185,12 @@ var ReactDOM = {
return renderSubtreeIntoContainer(null, element, container, callback);
},

unstable_renderSubtreeIntoContainer(parentComponent : ReactComponent<any, any, any>, element : ReactElement<any>, container : DOMContainerElement, callback: ?Function) {
unstable_renderSubtreeIntoContainer(parentComponent : ReactComponent<any, any, any>, element : ReactElement<any>, containerNode : DOMContainerElement | Document, callback: ?Function) {
invariant(
parentComponent != null && ReactInstanceMap.has(parentComponent),
'parentComponent must be a valid React Component'
);
return renderSubtreeIntoContainer(parentComponent, element, container, callback);
return renderSubtreeIntoContainer(parentComponent, element, containerNode, callback);
},

unmountComponentAtNode(container : DOMContainerElement) {
Expand Down
70 changes: 24 additions & 46 deletions src/renderers/noop/ReactNoop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@

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

var ReactFiberReconciler = require('ReactFiberReconciler');
var ReactInstanceMap = require('ReactInstanceMap');
Expand All @@ -32,59 +31,35 @@ var {
var scheduledAnimationCallback = null;
var scheduledDeferredCallback = null;

const TERMINAL_TAG = 99;
const TEXT_TAG = 98;

type Container = { rootID: string, children: Array<Instance | TextInstance> };
type Props = { prop: any };
type Instance = { tag: 99, type: string, id: number, children: Array<Instance | TextInstance>, prop: any };
type TextInstance = { tag: 98, text: string };
type Instance = {| type: string, id: number, children: Array<Instance | TextInstance>, prop: any |};
type TextInstance = {| text: string, id: number |};

var instanceCounter = 0;

function recursivelyAppendChildren(
flatArray : Array<Instance | TextInstance>,
child : HostChildren<Instance | TextInstance>
) {
if (!child) {
return;
}
if (child.tag === TERMINAL_TAG || child.tag === TEXT_TAG) {
flatArray.push(child);
} else {
let node = child;
do {
recursivelyAppendChildren(flatArray, node.output);
} while (node = node.sibling);
}
}

function flattenChildren(children : HostChildren<Instance | TextInstance>) {
const flatArray = [];
recursivelyAppendChildren(flatArray, children);
return flatArray;
}

var NoopRenderer = ReactFiberReconciler({

updateContainer(containerInfo : Container, children : HostChildren<Instance | TextInstance>) : void {
containerInfo.children = flattenChildren(children);
},

createInstance(type : string, props : Props, children : HostChildren<Instance | TextInstance>) : Instance {
createInstance(type : string, props : Props) : Instance {
const inst = {
tag: TERMINAL_TAG,
id: instanceCounter++,
type: type,
children: flattenChildren(children),
children: [],
prop: props.prop,
};
// Hide from unit tests
Object.defineProperty(inst, 'tag', { value: inst.tag, enumerable: false });
Object.defineProperty(inst, 'id', { value: inst.id, enumerable: false });
return inst;
},

appendInitialChild(parentInstance : Instance, child : Instance | TextInstance) : void {
parentInstance.children.push(child);
},

finalizeInitialChildren(domElement : Instance, type : string, props : Props) : void {
// Noop
},

prepareUpdate(instance : Instance, oldProps : Props, newProps : Props) : boolean {
return true;
},
Expand All @@ -94,17 +69,17 @@ var NoopRenderer = ReactFiberReconciler({
},

createTextInstance(text : string) : TextInstance {
var inst = { tag: TEXT_TAG, text : text };
var inst = { text : text, id: instanceCounter++ };
// Hide from unit tests
Object.defineProperty(inst, 'tag', { value: inst.tag, enumerable: false });
Object.defineProperty(inst, 'id', { value: inst.id, enumerable: false });
return inst;
},

commitTextUpdate(textInstance : TextInstance, oldText : string, newText : string) : void {
textInstance.text = newText;
},

appendChild(parentInstance : Instance, child : Instance | TextInstance) : void {
appendChild(parentInstance : Instance | Container, child : Instance | TextInstance) : void {
const index = parentInstance.children.indexOf(child);
if (index !== -1) {
parentInstance.children.splice(index, 1);
Expand All @@ -113,7 +88,7 @@ var NoopRenderer = ReactFiberReconciler({
},

insertBefore(
parentInstance : Instance,
parentInstance : Instance | Container,
child : Instance | TextInstance,
beforeChild : Instance | TextInstance
) : void {
Expand All @@ -128,7 +103,7 @@ var NoopRenderer = ReactFiberReconciler({
parentInstance.children.splice(beforeIndex, 0, child);
},

removeChild(parentInstance : Instance, child : Instance | TextInstance) : void {
removeChild(parentInstance : Instance | Container, child : Instance | TextInstance) : void {
const index = parentInstance.children.indexOf(child);
if (index === -1) {
throw new Error('This child does not exist.');
Expand Down Expand Up @@ -213,7 +188,7 @@ var ReactNoop = {
}
// Unsound duck typing.
const component = (componentOrElement : any);
if (component.tag === TERMINAL_TAG || component.tag === TEXT_TAG) {
if (typeof component.id === 'number') {
return component;
}
const inst = ReactInstanceMap.get(component);
Expand Down Expand Up @@ -278,10 +253,13 @@ var ReactNoop = {
function logHostInstances(children: Array<Instance | TextInstance>, depth) {
for (var i = 0; i < children.length; i++) {
var child = children[i];
if (child.tag === TEXT_TAG) {
log(' '.repeat(depth) + '- ' + child.text);
var indent = ' '.repeat(depth);
if (typeof child.text === 'string') {
log(indent + '- ' + child.text);
} else {
log(' '.repeat(depth) + '- ' + child.type + '#' + child.id);
// $FlowFixMe - The child should've been refined now.
log(indent + '- ' + child.type + '#' + child.id);
// $FlowFixMe - The child should've been refined now.
logHostInstances(child.children, depth + 1);
}
}
Expand Down
19 changes: 18 additions & 1 deletion src/renderers/shared/fiber/ReactFiberBeginWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,24 @@ module.exports = function<T, P, I, TI, C>(
}

function updatePortalComponent(current, workInProgress) {
reconcileChildren(current, workInProgress, workInProgress.pendingProps);
const priorityLevel = workInProgress.pendingWorkPriority;
const nextChildren = workInProgress.pendingProps;
if (!current) {
// Portals are special because we don't append the children during mount
// but at commit. Therefore we need to track insertions which the normal
// flow doesn't do during mount. This doesn't happen at the root because
// the root always starts with a "current" with a null child.
// TODO: Consider unifying this with how the root works.
Copy link
Collaborator

@gaearon gaearon Nov 24, 2016

Choose a reason for hiding this comment

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

How could this be unified?

Copy link
Collaborator Author

@sebmarkbage sebmarkbage Nov 24, 2016

Choose a reason for hiding this comment

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

This only happens to work because HostContainer always end up with two Fibers but that seems unnecessary.

Copy link
Collaborator Author

@sebmarkbage sebmarkbage Nov 24, 2016

Choose a reason for hiding this comment

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

So if we fix that, then this same logic will need to be applied for HostContainer too

workInProgress.child = reconcileChildFibersInPlace(
workInProgress,
workInProgress.child,
nextChildren,
priorityLevel
);
markChildAsProgressed(current, workInProgress, priorityLevel);
} else {
reconcileChildren(current, workInProgress, nextChildren);
}
}

/*
Expand Down
Loading