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

Remove _nextDom usage #4553

Merged
merged 1 commit into from
Nov 22, 2024
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion jsx-runtime/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ function createVNode(type, props, key, isStaticChildren, __source, __self) {
_parent: null,
_depth: 0,
_dom: null,
_nextDom: undefined,
_component: null,
constructor: undefined,
_original: --vnodeId,
Expand Down
5 changes: 0 additions & 5 deletions src/create-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,6 @@ export function createVNode(type, props, key, ref, original) {
_parent: null,
_depth: 0,
_dom: null,
// _nextDom must be initialized to undefined b/c it will eventually
// be set to dom.nextSibling which can return `null` and it is important
// to be able to distinguish between an uninitialized _nextDom and
// a _nextDom that has been set to `null`
_nextDom: UNDEFINED,
_component: null,
constructor: UNDEFINED,
_original: original == null ? ++vnodeId : original,
Expand Down
54 changes: 21 additions & 33 deletions src/diff/children.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,12 @@ export function diffChildren(

let newChildrenLength = renderResult.length;

newParentVNode._nextDom = oldDom;
constructNewChildrenArray(newParentVNode, renderResult, oldChildren);
oldDom = newParentVNode._nextDom;
oldDom = constructNewChildrenArray(
newParentVNode,
renderResult,
oldChildren,
oldDom
);

for (i = 0; i < newChildrenLength; i++) {
childVNode = newParentVNode._children[i];
Expand All @@ -82,7 +85,7 @@ export function diffChildren(
childVNode._index = i;

// Morph the old element into the new one, but don't append it to the dom yet
diff(
let result = diff(
parentDom,
childVNode,
oldVNode,
Expand Down Expand Up @@ -117,49 +120,32 @@ export function diffChildren(
oldVNode._children === childVNode._children
) {
oldDom = insert(childVNode, oldDom, parentDom);
} else if (
typeof childVNode.type == 'function' &&
childVNode._nextDom !== UNDEFINED
) {
// Since Fragments or components that return Fragment like VNodes can
// contain multiple DOM nodes as the same level, continue the diff from
// the sibling of last DOM child of this child VNode
oldDom = childVNode._nextDom;
} else if (typeof childVNode.type == 'function' && result !== UNDEFINED) {
oldDom = result;
} else if (newDom) {
oldDom = newDom.nextSibling;
}

// Eagerly cleanup _nextDom. We don't need to persist the value because it
// is only used by `diffChildren` to determine where to resume the diff
// after diffing Components and Fragments. Once we store it the nextDOM
// local var, we can clean up the property. Also prevents us hanging on to
// DOM nodes that may have been unmounted.
childVNode._nextDom = UNDEFINED;

// Unset diffing flags
childVNode._flags &= ~(INSERT_VNODE | MATCHED);
}

// TODO: With new child diffing algo, consider alt ways to diff Fragments.
// Such as dropping oldDom and moving fragments in place
//
// Because the newParentVNode is Fragment-like, we need to set it's
// _nextDom property to the nextSibling of its last child DOM node.
//
// `oldDom` contains the correct value here because if the last child
// is a Fragment-like, then oldDom has already been set to that child's _nextDom.
// If the last child is a DOM VNode, then oldDom will be set to that DOM
// node's nextSibling.
newParentVNode._nextDom = oldDom;
newParentVNode._dom = firstChildDom;

return oldDom;
}

/**
* @param {VNode} newParentVNode
* @param {ComponentChildren[]} renderResult
* @param {VNode[]} oldChildren
*/
function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) {
function constructNewChildrenArray(
newParentVNode,
renderResult,
oldChildren,
oldDom
) {
/** @type {number} */
let i;
/** @type {VNode} */
Expand Down Expand Up @@ -309,14 +295,16 @@ function constructNewChildrenArray(newParentVNode, renderResult, oldChildren) {
for (i = 0; i < oldChildrenLength; i++) {
oldVNode = oldChildren[i];
if (oldVNode != null && (oldVNode._flags & MATCHED) === 0) {
if (oldVNode._dom == newParentVNode._nextDom) {
newParentVNode._nextDom = getDomSibling(oldVNode);
if (oldVNode._dom == oldDom) {
oldDom = getDomSibling(oldVNode);
}

unmount(oldVNode, oldVNode);
}
}
}

return oldDom;
}

/**
Expand Down
13 changes: 6 additions & 7 deletions src/diff/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ export function diff(
tmp != null && tmp.type === Fragment && tmp.key == null;
let renderResult = isTopLevelFragment ? tmp.props.children : tmp;

diffChildren(
oldDom = diffChildren(
parentDom,
isArray(renderResult) ? renderResult : [renderResult],
newVNode,
Expand Down Expand Up @@ -281,6 +281,7 @@ export function diff(
while (oldDom && oldDom.nodeType === 8 && oldDom.nextSibling) {
oldDom = oldDom.nextSibling;
}

excessDomChildren[excessDomChildren.indexOf(oldDom)] = null;
newVNode._dom = oldDom;
} else {
Expand All @@ -296,7 +297,7 @@ export function diff(
newVNode._children = oldVNode._children;
newVNode._dom = oldVNode._dom;
} else {
newVNode._dom = diffElementNodes(
oldDom = newVNode._dom = diffElementNodes(
oldVNode._dom,
newVNode,
oldVNode,
Expand All @@ -310,6 +311,8 @@ export function diff(
}

if ((tmp = options.diffed)) tmp(newVNode);

return newVNode._flags & MODE_SUSPENDED ? undefined : oldDom;
}

/**
Expand All @@ -318,8 +321,6 @@ export function diff(
* @param {VNode} root
*/
export function commitRoot(commitQueue, root, refQueue) {
root._nextDom = UNDEFINED;

for (let i = 0; i < refQueue.length; i++) {
applyRef(refQueue[i], refQueue[++i], refQueue[++i]);
}
Expand Down Expand Up @@ -632,9 +633,7 @@ export function unmount(vnode, parentVNode, skipRemove) {
removeNode(vnode._dom);
}

// Must be set to `undefined` to properly clean up `_nextDom`
// for which `null` is a valid value. See comment in `create-element.js`
vnode._component = vnode._parent = vnode._dom = vnode._nextDom = UNDEFINED;
vnode._component = vnode._parent = vnode._dom = UNDEFINED;
}

/** The `.render()` method for a PFC backing instance. */
Expand Down
4 changes: 0 additions & 4 deletions src/internal.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -150,10 +150,6 @@ declare global {
* The [first (for Fragments)] DOM child of a VNode
*/
_dom: PreactElement | null;
/**
* The last dom child of a Fragment, or components that return a Fragment
*/
_nextDom: PreactElement | null | undefined;
_component: Component | null;
constructor: undefined;
_original: number;
Expand Down