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

Flatten children array as it is diffed #1716

Merged
merged 3 commits into from
Sep 9, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
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
32 changes: 17 additions & 15 deletions src/diff/children.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,8 @@ import { getDomSibling } from '../component';
* @param {boolean} isHydrating Whether or not we are in hydration
*/
export function diffChildren(parentDom, newParentVNode, oldParentVNode, context, isSvg, excessDomChildren, mounts, oldDom, isHydrating) {
let childVNode, i, j, oldVNode, newDom, sibDom, firstChildDom, refs;
let i, j, oldVNode, newDom, sibDom, firstChildDom, refs;

let newChildren = newParentVNode._children || toChildArray(newParentVNode.props.children, newParentVNode._children=[], coerceToVNode, true);
// This is a compression of oldParentVNode!=null && oldParentVNode != EMPTY_OBJ && oldParentVNode._children || EMPTY_ARR
// as EMPTY_OBJ._children should be `undefined`.
let oldChildren = (oldParentVNode && oldParentVNode._children) || EMPTY_ARR;
Expand All @@ -49,8 +48,8 @@ export function diffChildren(parentDom, newParentVNode, oldParentVNode, context,
}
}

for (i=0; i<newChildren.length; i++) {
childVNode = newChildren[i] = coerceToVNode(newChildren[i]);
i=0;
newParentVNode._children = toChildArray(newParentVNode._children, childVNode => {
marvinhagemeister marked this conversation as resolved.
Show resolved Hide resolved

if (childVNode!=null) {
childVNode._parent = newParentVNode;
Expand Down Expand Up @@ -150,7 +149,10 @@ export function diffChildren(parentDom, newParentVNode, oldParentVNode, context,
}
}
}
}

i++;
return childVNode;
});

newParentVNode._dom = firstChildDom;

Expand All @@ -169,27 +171,27 @@ export function diffChildren(parentDom, newParentVNode, oldParentVNode, context,
}

/**
* Flatten a virtual nodes children to a single dimensional array
* Flatten and loop through the children of a virtual node
* @param {import('../index').ComponentChildren} children The unflattened
* children of a virtual node
* @param {Array<import('../internal').VNode | null>} [flattened] An flat array of children to modify
* @param {typeof import('../create-element').coerceToVNode} [map] Function that
* will be applied on each child if the `vnode` is not `null`
* @param {boolean} [keepHoles] wether to coerce `undefined` to `null` or not.
* This is needed for Components without children like `<Foo />`.
* @param {(vnode: import('../internal').VNode) => import('../internal').VNode} [callback]
* A function to invoke for each child before it is added to the flattened list.
* @param {import('../internal').VNode[]} [flattened] An flat array of children to modify
* @returns {import('../internal').VNode[]}
*/
export function toChildArray(children, flattened, map, keepHoles) {
export function toChildArray(children, callback, flattened) {
Copy link
Member

Choose a reason for hiding this comment

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

IIRC the first two args here are used externally. Maybe worth changing to toChildArray(children, flattened, callback)?

Copy link
Member

Choose a reason for hiding this comment

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

I think that change was made for golfing purposes in children.js. There it's used without the flattened argument. Either way we've only advertised the first arg so far in our types and elsewhere. So I think it's fine to swap them like it's done here.

if (flattened == null) flattened = [];

if (children==null || typeof children === 'boolean') {
if (keepHoles) flattened.push(null);
if (callback) flattened.push(callback(null));
}
else if (Array.isArray(children)) {
for (let i=0; i < children.length; i++) {
toChildArray(children[i], flattened, map, keepHoles);
toChildArray(children[i], callback, flattened);
}
}
else {
flattened.push(map ? map(children) : children);
flattened.push(callback ? callback(coerceToVNode(children)) : children);
}

return flattened;
Expand Down
8 changes: 5 additions & 3 deletions src/diff/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { EMPTY_OBJ, EMPTY_ARR } from '../constants';
import { Component, enqueueRender } from '../component';
import { coerceToVNode, Fragment } from '../create-element';
import { diffChildren, toChildArray } from './children';
import { Fragment } from '../create-element';
import { diffChildren } from './children';
import { diffProps } from './props';
import { assign, removeNode } from '../util';
import options from '../options';
Expand Down Expand Up @@ -115,7 +115,7 @@ export function diff(parentDom, newVNode, oldVNode, context, isSvg, excessDomChi

tmp = c.render(c.props, c.state, c.context);
let isTopLevelFragment = tmp != null && tmp.type == Fragment && tmp.key == null;
toChildArray(isTopLevelFragment ? tmp.props.children : tmp, newVNode._children=[], coerceToVNode, true);
newVNode._children = isTopLevelFragment ? tmp.props.children : tmp;

if (c.getChildContext!=null) {
context = assign(assign({}, context), c.getChildContext());
Expand Down Expand Up @@ -239,6 +239,8 @@ function diffElementNodes(dom, newVNode, oldVNode, context, isSvg, excessDomChil

diffProps(dom, newProps, oldProps, isSvg, isHydrating);

newVNode._children = newVNode.props.children;
marvinhagemeister marked this conversation as resolved.
Show resolved Hide resolved

// If the new vnode didn't have dangerouslySetInnerHTML, diff its children
if (!newHtml) {
diffChildren(dom, newVNode, oldVNode, context, newVNode.type==='foreignObject' ? false : isSvg, excessDomChildren, mounts, EMPTY_OBJ, isHydrating);
Expand Down