From e572679749416ad5e58788f4f2445e532afb0567 Mon Sep 17 00:00:00 2001 From: Robby6Strings Date: Sun, 30 Jun 2024 18:02:35 +1200 Subject: [PATCH] bugfix: internal refactoring to improve reconciliation & better handle reordering of children --- packages/lib/src/constants.ts | 5 +- packages/lib/src/context.ts | 3 +- packages/lib/src/hooks/useContext.ts | 3 +- packages/lib/src/index.ts | 31 +- packages/lib/src/memo.ts | 7 +- packages/lib/src/props.ts | 2 +- packages/lib/src/reconciler.ts | 273 +++++++++++------- packages/lib/src/router.ts | 6 +- packages/lib/src/scheduler.ts | 45 ++- packages/lib/src/ssr/server.ts | 10 +- packages/lib/src/types.ts | 3 +- sandbox/csr/src/App.tsx | 48 ++- .../csr/src/components/UseAsyncExample.tsx | 5 +- 13 files changed, 251 insertions(+), 190 deletions(-) diff --git a/packages/lib/src/constants.ts b/packages/lib/src/constants.ts index de6d4e47..b76fb192 100644 --- a/packages/lib/src/constants.ts +++ b/packages/lib/src/constants.ts @@ -1,4 +1,6 @@ export const SignalKey = Symbol.for("kaioken.signal") +export const componentSymbol = Symbol.for("kaioken.component") +export const contextDataSymbol = Symbol.for("kaioken.contextData") export const EffectTag = { UPDATE: 1, @@ -6,9 +8,6 @@ export const EffectTag = { DELETION: 3, } as const -export const componentSymbol = Symbol.for("kaioken.component") -export const elementFreezeSymbol = Symbol.for("kaioken.freezeElement") - export const elementTypes = { text: "#text", fragment: "KAIOKEN_FRAGMENT", diff --git a/packages/lib/src/context.ts b/packages/lib/src/context.ts index fea7b036..06092f81 100644 --- a/packages/lib/src/context.ts +++ b/packages/lib/src/context.ts @@ -1,7 +1,6 @@ +import { contextDataSymbol } from "./constants.js" import { fragment } from "./index.js" -const contextDataSymbol = Symbol.for("kaioken.contextData") - export function createContext(defaultValue: T): Kaioken.Context { const ctx = { Provider: ({ value, children = [] }: Kaioken.ProviderProps) => { diff --git a/packages/lib/src/hooks/useContext.ts b/packages/lib/src/hooks/useContext.ts index ac358ae3..dc6037fa 100644 --- a/packages/lib/src/hooks/useContext.ts +++ b/packages/lib/src/hooks/useContext.ts @@ -1,6 +1,5 @@ import { useHook } from "./utils.js" - -const contextDataSymbol = Symbol.for("kaioken.contextData") +import { contextDataSymbol } from "../constants.js" type ContextNode = Kaioken.VNode & { props: { diff --git a/packages/lib/src/index.ts b/packages/lib/src/index.ts index 1b37b3db..2a72f3ba 100644 --- a/packages/lib/src/index.ts +++ b/packages/lib/src/index.ts @@ -1,6 +1,5 @@ import { AppContext, AppContextOptions } from "./appContext.js" import { - isValidChild, isVNode, propFilters, propToHtmlAttr, @@ -67,34 +66,22 @@ function mount>( function createElement( type: string | Function | typeof Component, - props = {}, - ...children: JSX.Element[] + props: null | Record = null, + ...children: unknown[] ): VNode { - const node = { + const node: VNode = { type, index: 0, - props: { - ...props, - children: children.flat().filter(isValidChild).map(createChildElement), - }, + props: children.length ? { ...props, children } : props ?? {}, } nodeToCtxMap.set(node, ctx.current) return node } -function createChildElement(child: JSX.Element): VNode { - if (isVNode(child)) return child - return createTextElement(String(child)) -} - -function createTextElement(nodeValue: string): VNode { - return createElement(et.text, { nodeValue }) -} - function fragment({ children, ...rest -}: { children: JSX.Element[] } & Record) { +}: { children: unknown[] } & Record) { return createElement(et.fragment, rest, ...children) } @@ -113,7 +100,7 @@ function renderToString>( } function renderToString_internal>( - el: JSX.Element, + el: unknown, parent?: VNode | undefined, elProps = {} as T ): string { @@ -121,7 +108,8 @@ function renderToString_internal>( if (el === undefined) return "" if (typeof el === "boolean") return "" if (typeof el === "string") return encodeHtmlEntities(el) - if (typeof el === "number") return encodeHtmlEntities(el.toString()) + if (typeof el === "number" || typeof el === "bigint") + return encodeHtmlEntities(el.toString()) if (typeof el === "function") return renderToString_internal(createElement(el, elProps)) if (el instanceof Array) { @@ -132,6 +120,7 @@ function renderToString_internal>( return s } if (Signal.isSignal(el)) return encodeHtmlEntities(el.value.toString()) + if (!isVNode(el)) return String(el) el.parent = parent nodeToCtxMap.set(el, ctx.current) @@ -168,7 +157,7 @@ function renderToString_internal>( ? Signal.isSignal(props.innerHTML) ? props.innerHTML.value : props.innerHTML - : children.map((c) => renderToString_internal(c, el, c.props)).join("") + : children.map((c) => renderToString_internal(c, el)).join("") return `<${type}${attrs.length ? " " + attrs : ""}${isSelfClosing ? "/>" : `>${inner}`}` } diff --git a/packages/lib/src/memo.ts b/packages/lib/src/memo.ts index c98e888c..1f62e1ff 100644 --- a/packages/lib/src/memo.ts +++ b/packages/lib/src/memo.ts @@ -1,4 +1,3 @@ -import { elementFreezeSymbol } from "./constants.js" import { createElement } from "./index.js" type Rec = Record @@ -20,7 +19,8 @@ export function memo>( return Object.assign( (props: Props) => { if (node && arePropsEqual(oldProps, props)) { - return Object.assign(node, { [elementFreezeSymbol]: true }) + node.frozen = true + return node } oldProps = props if (!node) { @@ -28,7 +28,8 @@ export function memo>( } else { Object.assign(node.props, props) } - return Object.assign(node, { [elementFreezeSymbol]: false }) + node.frozen = false + return node }, { displayName: "Kaioken.memo" } ) diff --git a/packages/lib/src/props.ts b/packages/lib/src/props.ts index 7d14e5c1..e682ac63 100644 --- a/packages/lib/src/props.ts +++ b/packages/lib/src/props.ts @@ -1,5 +1,5 @@ export function assertValidElementProps(vNode: Kaioken.VNode) { - if (vNode.props.children.length && vNode.props.innerHTML) { + if (vNode.props.children?.length && vNode.props.innerHTML) { throw new Error( "[kaioken]: Cannot use both children and innerHTML on an element" ) diff --git a/packages/lib/src/reconciler.ts b/packages/lib/src/reconciler.ts index c83951f0..63deb5dc 100644 --- a/packages/lib/src/reconciler.ts +++ b/packages/lib/src/reconciler.ts @@ -1,64 +1,72 @@ -import { EffectTag, elementTypes, elementFreezeSymbol } from "./constants.js" -import type { AppContext } from "./appContext" +import { EffectTag, elementTypes } from "./constants.js" import { ctx, nodeToCtxMap } from "./globals.js" -import { isVNode, isValidChild } from "./utils.js" +import { isVNode } from "./utils.js" +import { createElement } from "./index.js" type VNode = Kaioken.VNode export function reconcileChildren( - appCtx: AppContext, vNode: VNode, - children: VNode[] + currentFirstChild: VNode | null, + children: unknown[] ) { - let index = 0 - let prevOldNode: VNode | undefined = undefined - let oldNode: VNode | undefined = vNode.prev && vNode.prev.child - let prevNewNode: VNode | undefined = undefined - let newNode: VNode | undefined = undefined - let resultingChild: VNode | undefined = undefined - let lastPlacedIndex = 0 + // if (vNode.type instanceof Function && "test" in vNode.type) { + // debugger + // } + let resultingChild: VNode | null = null + let prevNewNode: VNode | null = null - for (; !!oldNode && index < children.length; index++) { - newNode = updateSlot(appCtx, vNode, children[index], oldNode, index) - if (!newNode) break - lastPlacedIndex = placeChild(newNode, lastPlacedIndex, index) + let oldNode = currentFirstChild + let lastPlacedIndex = 0 + let newIdx = 0 + let nextOldNode = null - if (oldNode) { - prevOldNode = oldNode - oldNode = oldNode.sibling + for (; !!oldNode && newIdx < children.length; newIdx++) { + if (oldNode.index > newIdx) { + nextOldNode = oldNode + oldNode = null + } else { + nextOldNode = oldNode.sibling || null } - if (prevNewNode) { - prevNewNode.sibling = newNode + const newNode = updateSlot(vNode, oldNode, children[newIdx]) + if (newNode === null) { + if (oldNode === null) { + oldNode = nextOldNode + } + break } - prevNewNode = newNode - if (index === 0) { + if (oldNode && !newNode.prev) { + ctx.current.requestDelete(oldNode) + } + lastPlacedIndex = placeChild(newNode, lastPlacedIndex, newIdx) + if (prevNewNode === null) { resultingChild = newNode + } else { + prevNewNode.sibling = newNode } + prevNewNode = newNode + oldNode = nextOldNode } // matched all children? - if (index === children.length) { - while (oldNode) { - if (prevOldNode) { - prevOldNode.sibling = undefined - } - prevOldNode = oldNode - appCtx.requestDelete(oldNode) - oldNode = oldNode.sibling + if (newIdx === children.length) { + while (oldNode !== null) { + ctx.current.requestDelete(oldNode) + oldNode = oldNode.sibling || null } return resultingChild } // just some good ol' insertions, baby - if (!oldNode) { - for (; index < children.length; index++) { - const child = children[index] - if (!isValidChild(child)) continue - newNode = createChild(vNode, child, index) - lastPlacedIndex = placeChild(newNode, lastPlacedIndex, index) - if (index === 0) { + if (oldNode === null) { + for (; newIdx < children.length; newIdx++) { + const newNode = createChild(vNode, children[newIdx]) + if (newNode === null) continue + + lastPlacedIndex = placeChild(newNode, lastPlacedIndex, newIdx) + if (prevNewNode === null) { resultingChild = newNode - } else if (prevNewNode) { + } else { prevNewNode.sibling = newNode } prevNewNode = newNode @@ -69,10 +77,14 @@ export function reconcileChildren( // deal with mismatched keys / unmatched children const existingChildren = mapRemainingChildren(oldNode) - for (; index < children.length; index++) { - const child = children[index] - const newNode = updateFromMap(existingChildren, vNode, index, child) - if (newNode !== undefined) { + for (; newIdx < children.length; newIdx++) { + const newNode = updateFromMap( + existingChildren, + vNode, + newIdx, + children[newIdx] + ) + if (newNode !== null) { if (newNode.prev !== undefined) { // node persisted, remove it from the list so it doesn't get deleted existingChildren.delete( @@ -81,87 +93,130 @@ export function reconcileChildren( : newNode.prev.props.key ) } - copyNodeMemoization(child, newNode) - lastPlacedIndex = placeChild(newNode, lastPlacedIndex, index) - nodeToCtxMap.set(newNode, ctx.current) - - if (index === 0) { + lastPlacedIndex = placeChild(newNode, lastPlacedIndex, newIdx) + if (prevNewNode === null) { resultingChild = newNode - } else if (prevNewNode) { + } else { prevNewNode.sibling = newNode } prevNewNode = newNode - if (index === children.length - 1) { - newNode.sibling = undefined - } } } - existingChildren.forEach((child) => appCtx.requestDelete(child)) + existingChildren.forEach((child) => ctx.current.requestDelete(child)) return resultingChild } -function createChild(parent: VNode, child: any, index: number): VNode { - let newNode: VNode +function updateSlot(parent: VNode, oldNode: VNode | null, child: unknown) { + // Update the node if the keys match, otherwise return undefined. + const key = oldNode !== null ? oldNode.props.key : undefined + if ( + (typeof child === "string" && child !== "") || + typeof child === "number" || + typeof child === "bigint" + ) { + if (key !== undefined) return null + return updateTextNode(parent, oldNode, "" + child) + } if (isVNode(child)) { - newNode = { - type: child.type, - props: child.props, - parent, - effectTag: EffectTag.PLACEMENT, - index, - } - } else { - newNode = { - type: elementTypes.text, - props: { - nodeValue: String(child), - children: [], - }, - parent, - effectTag: EffectTag.PLACEMENT, - index, - } + if (child.props.key !== key) return null + return updateNode(parent, oldNode, child) } + if (Array.isArray(child)) { + if (key !== undefined) return null + return updateFragment(parent, oldNode, child) + } + return null +} - copyNodeMemoization(child, newNode) - nodeToCtxMap.set(newNode, ctx.current) +function updateTextNode(parent: VNode, oldNode: VNode | null, content: string) { + if (oldNode === null || oldNode.type !== elementTypes.text) { + const newNode = createElement(elementTypes.text, { nodeValue: content }) + newNode.parent = parent + return newNode + } else { + const newNode = oldNode + newNode.prev = { ...oldNode, props: { ...oldNode.props }, prev: undefined } + newNode.props.nodeValue = content + newNode.effectTag = EffectTag.UPDATE + newNode.sibling = undefined + return oldNode + } +} - return newNode +function updateNode(parent: VNode, oldNode: VNode | null, newNode: VNode) { + const nodeType = newNode.type + if (nodeType === elementTypes.fragment) { + return updateFragment( + parent, + oldNode, + newNode.props.children || [], + newNode.props + ) + } + if (oldNode?.type === nodeType) { + oldNode.prev = { ...oldNode, props: { ...oldNode.props }, prev: undefined } + oldNode.index = 0 + oldNode.props = newNode.props + oldNode.parent = parent + oldNode.sibling = undefined + oldNode.effectTag = EffectTag.UPDATE + oldNode.frozen = newNode.frozen + return oldNode + } + const created = createElement(nodeType, newNode.props) + created.parent = parent + return created } -function updateSlot( - appCtx: AppContext, +function updateFragment( parent: VNode, - child: VNode, - oldNode: VNode | undefined, - index: number + oldNode: VNode | null, + children: unknown[], + newProps = {} ) { - let newNode: VNode | undefined - const sameType = oldNode && child && child.type == oldNode.type - if (oldNode && child && child.type == oldNode.type) { - if (oldNode.props.key !== child.props.key) return undefined - newNode = oldNode - newNode.props = child.props - newNode.parent = parent - newNode.effectTag = EffectTag.UPDATE - copyNodeMemoization(child, newNode) - nodeToCtxMap.set(newNode, ctx.current) - } else if (isValidChild(child) && !sameType) { - newNode = createChild(parent, child, index) - } - if (oldNode && !sameType) { - appCtx.requestDelete(oldNode) + if (oldNode === null || oldNode.type !== elementTypes.fragment) { + const el = createElement(elementTypes.fragment, { children }) + el.parent = parent + return el } - return newNode + oldNode.prev = { ...oldNode, props: { ...oldNode.props }, prev: undefined } + oldNode.props = { ...newProps, children } + oldNode.parent = parent + oldNode.effectTag = EffectTag.UPDATE + oldNode.sibling = undefined + return oldNode } -function copyNodeMemoization(child: VNode, vNode: VNode) { - if (elementFreezeSymbol in child) { - Object.assign(vNode, { - [elementFreezeSymbol]: child[elementFreezeSymbol], - }) +function createChild(parent: VNode, child: unknown): VNode | null { + if ( + (typeof child === "string" && child !== "") || + typeof child === "number" || + typeof child === "bigint" + ) { + const el = createElement(elementTypes.text, { nodeValue: "" + child }) + el.parent = parent + return el + } + + if (typeof child === "object" && child !== null) { + if (isVNode(child)) { + const newNode: VNode = { + type: child.type, + props: child.props, + parent, + effectTag: EffectTag.PLACEMENT, + index: 0, + } + nodeToCtxMap.set(newNode, ctx.current) + return newNode + } else if (Array.isArray(child)) { + const el = createElement(elementTypes.fragment, { children: child }) + el.parent = parent + return el + } } + return null } function placeChild( @@ -190,13 +245,19 @@ function updateFromMap( parent: VNode, index: number, newChild: any -): VNode | undefined { +): VNode | null { if ( (typeof newChild === "string" && newChild !== "") || - typeof newChild === "number" + typeof newChild === "number" || + typeof newChild === "bigint" ) { const oldChild = existingChildren.get(index) if (oldChild) { + oldChild.prev = { + ...oldChild, + props: { ...oldChild.props }, + prev: undefined, + } oldChild.effectTag = EffectTag.UPDATE oldChild.props.nodeValue = newChild return oldChild @@ -221,6 +282,8 @@ function updateFromMap( if (oldChild) { oldChild.effectTag = EffectTag.UPDATE oldChild.props = newChild.props + oldChild.frozen = newChild.frozen + oldChild.sibling = undefined return oldChild } else { return { @@ -233,7 +296,7 @@ function updateFromMap( } } - return + return null } function mapRemainingChildren(vNode: VNode) { diff --git a/packages/lib/src/router.ts b/packages/lib/src/router.ts index 60df05e6..83a1452a 100644 --- a/packages/lib/src/router.ts +++ b/packages/lib/src/router.ts @@ -98,7 +98,11 @@ function Router(props: RouterProps) { } if (fallbackRoute) { - return createElement(fallbackRoute.props.element, { params: {}, query }) + return fragment({ + children: [ + createElement(fallbackRoute.props.element, { params: {}, query }), + ], + }) } return null diff --git a/packages/lib/src/scheduler.ts b/packages/lib/src/scheduler.ts index a0d0e208..a505a6dc 100644 --- a/packages/lib/src/scheduler.ts +++ b/packages/lib/src/scheduler.ts @@ -1,10 +1,6 @@ import type { AppContext } from "./appContext" import { Component } from "./component.js" -import { - EffectTag, - elementFreezeSymbol, - elementTypes as et, -} from "./constants.js" +import { EffectTag, elementTypes as et } from "./constants.js" import { commitWork, createDom, updateDom } from "./dom.js" import { childIndexStack, @@ -197,8 +193,7 @@ export class Scheduler { } private performUnitOfWork(vNode: VNode): VNode | void { - const frozen = - elementFreezeSymbol in vNode && vNode[elementFreezeSymbol] === true + const frozen = "frozen" in vNode && vNode.frozen === true const skip = frozen && vNode.effectTag !== EffectTag.PLACEMENT if (!skip) { try { @@ -207,11 +202,12 @@ export class Scheduler { } else if (vNode.type instanceof Function) { this.updateFunctionComponent(vNode) } else if (vNode.type === et.fragment) { - vNode.child = reconcileChildren( - this.appCtx, - vNode, - vNode.props.children - ) + vNode.child = + reconcileChildren( + vNode, + vNode.child || null, + vNode.props.children || [] + ) || undefined } else { this.updateHostComponent(vNode) } @@ -256,11 +252,10 @@ export class Scheduler { vNode.instance.props = vNode.props } - vNode.child = reconcileChildren( - this.appCtx, - vNode, - [vNode.instance.render()].flat() as VNode[] - ) + vNode.child = + reconcileChildren(vNode, vNode.child || null, [ + vNode.instance.render(), + ] as VNode[]) || undefined this.queueCurrentNodeEffects() node.current = undefined } @@ -268,11 +263,10 @@ export class Scheduler { private updateFunctionComponent(vNode: VNode) { this.appCtx.hookIndex = 0 node.current = vNode - vNode.child = reconcileChildren( - this.appCtx, - vNode, - [(vNode.type as Function)(vNode.props)].flat() - ) + vNode.child = + reconcileChildren(vNode, vNode.child || null, [ + (vNode.type as Function)(vNode.props), + ]) || undefined this.queueCurrentNodeEffects() node.current = undefined } @@ -300,7 +294,12 @@ export class Scheduler { if (vNode.props.ref) { vNode.props.ref.current = vNode.dom } - vNode.child = reconcileChildren(this.appCtx, vNode, vNode.props.children) + vNode.child = + reconcileChildren( + vNode, + vNode.child || null, + vNode.props.children || [] + ) || undefined } } function handleTextNodeSplitting(vNode: VNode) { diff --git a/packages/lib/src/ssr/server.ts b/packages/lib/src/ssr/server.ts index 8e3efb25..eaf5fc6a 100644 --- a/packages/lib/src/ssr/server.ts +++ b/packages/lib/src/ssr/server.ts @@ -5,6 +5,7 @@ import { renderMode, ctx, node, contexts, nodeToCtxMap } from "../globals.js" import { encodeHtmlEntities, + isVNode, propFilters, propToHtmlAttr, propValueToHtmlAttrValue, @@ -44,7 +45,7 @@ export function renderToReadableStream>( function renderToStream_internal>( state: RequestState, - el: JSX.Element, + el: unknown, parent?: Kaioken.VNode | undefined, elProps = {} as T ): void { @@ -71,6 +72,11 @@ function renderToStream_internal>( return } + if (!isVNode(el)) { + state.stream.push(String(el)) + return + } + el.parent = parent nodeToCtxMap.set(el, ctx.current) const props = el.props ?? {} @@ -119,7 +125,7 @@ function renderToStream_internal>( ) ) } else { - children.forEach((c) => renderToStream_internal(state, c, el, c.props)) + children.forEach((c) => renderToStream_internal(state, c, el)) } state.stream.push(``) diff --git a/packages/lib/src/types.ts b/packages/lib/src/types.ts index b5844350..9f98172d 100644 --- a/packages/lib/src/types.ts +++ b/packages/lib/src/types.ts @@ -101,7 +101,7 @@ declare global { instance?: Component props: { [key: string]: any - children: VNode[] + children?: unknown[] key?: JSX.ElementKey ref?: Kaioken.Ref } @@ -113,6 +113,7 @@ declare global { sibling?: VNode prev?: VNode effectTag?: (typeof EffectTag)[keyof typeof EffectTag] + frozen?: boolean } } } diff --git a/sandbox/csr/src/App.tsx b/sandbox/csr/src/App.tsx index 28e8e246..f98d5224 100644 --- a/sandbox/csr/src/App.tsx +++ b/sandbox/csr/src/App.tsx @@ -1,4 +1,4 @@ -import { Router, Route, useMemo } from "kaioken" +import { Router, Route, useState } from "kaioken" import { Todos } from "./components/ToDos" import { Counter } from "./components/Counter" import { ProductPage } from "./components/Product" @@ -12,7 +12,6 @@ import { Transitions } from "./components/Transitions" import { KeyedList } from "./components/KeyedList" import { ContextExample } from "./components/ContextExample" import { UseAsyncExample } from "./components/UseAsyncExample" -import { count, todo } from "./components/signals/test" function Nav() { return ( @@ -38,35 +37,34 @@ function Nav() { ) } -export function App() { - const double = useMemo(() => { - return count.value * 2 - }, [count.value]) +function Test() { + const [show, setShow] = useState(false) + return ( +
+ + {show &&
test
} + + +
+ ) +} +function Cntr() { + const [count, setCount] = useState(0) + return ( +
+
{count}
+ +
+ ) +} +export function App() { return ( <>