From 859748aed98c21fe8b8262f36a239eebfdd1912c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= Date: Fri, 1 Apr 2022 12:04:03 +0200 Subject: [PATCH] [FIX] concurrency issue (more robust handling of children per render) Before this commit, the list of all children was managed at the level of the root fiber, but this could cause issue when subfibers would be reused. With this commit, we use the childrenMap object that exists on each fiber instead. --- src/app/app.ts | 2 +- src/component/component_node.ts | 16 ++- src/component/fibers.ts | 7 +- .../__snapshots__/concurrency.test.ts.snap | 63 +++++++++++ tests/components/concurrency.test.ts | 100 ++++++++++++++++++ 5 files changed, 176 insertions(+), 12 deletions(-) diff --git a/src/app/app.ts b/src/app/app.ts index ce7fd846..a4ea1f9e 100644 --- a/src/app/app.ts +++ b/src/app/app.ts @@ -67,7 +67,7 @@ export class App< } makeNode(Component: ComponentConstructor, props: any): ComponentNode { - return new ComponentNode(Component, props, this); + return new ComponentNode(Component, props, this, null, null); } mountNode(node: ComponentNode, target: HTMLElement, options?: MountOptions) { diff --git a/src/component/component_node.ts b/src/component/component_node.ts index c82e6129..d35c1197 100644 --- a/src/component/component_node.ts +++ b/src/component/component_node.ts @@ -109,13 +109,11 @@ export function component

( throw new Error(`Cannot find the definition of component "${name}"`); } } - node = new ComponentNode(C, props, ctx.app, ctx); + node = new ComponentNode(C, props, ctx.app, ctx, key); ctx.children[key] = node; node.initiateRender(new Fiber(node, parentFiber)); } parentFiber.childrenMap[key] = node; - - parentFiber.root!.reachedChildren.add(node); return node; } @@ -133,6 +131,7 @@ export class ComponentNode

implements VNode implements VNode, props: P, app: App, parent?: ComponentNode) { + constructor( + C: ComponentConstructor, + props: P, + app: App, + parent: ComponentNode | null, + parentKey: string | null + ) { currentNode = this; this.app = app; - this.parent = parent || null; + this.parent = parent; + this.parentKey = parentKey; this.level = parent ? parent.level + 1 : 0; applyDefaultProps(props, C); const env = (parent && parent.childEnv) || app.env; diff --git a/src/component/fibers.ts b/src/component/fibers.ts index 0ff9eb6f..79a43ff0 100644 --- a/src/component/fibers.ts +++ b/src/component/fibers.ts @@ -25,9 +25,6 @@ export function makeRootFiber(node: ComponentNode): Fiber { current.children = []; current.childrenMap = {}; current.bdom = null; - if (current === root) { - root.reachedChildren = new WeakSet(); - } if (fibersInError.has(current)) { fibersInError.delete(current); fibersInError.delete(root); @@ -108,7 +105,7 @@ export class Fiber { scheduler.delayedRenders.push(this); return; } else { - if (!root.reachedChildren.has(prev)) { + if (!(prev.parentKey! in current.fiber.childrenMap)) { // is dead. but we keep the render around just in case scheduler.delayedRenders.push(this); return; @@ -150,8 +147,6 @@ export class RootFiber extends Fiber { // i.e.: render triggered in onWillUnmount or in willPatch will be delayed locked: boolean = false; - reachedChildren: WeakSet = new WeakSet(); - complete() { const node = this.node; this.locked = true; diff --git a/tests/components/__snapshots__/concurrency.test.ts.snap b/tests/components/__snapshots__/concurrency.test.ts.snap index 3b1bee88..0c29a47b 100644 --- a/tests/components/__snapshots__/concurrency.test.ts.snap +++ b/tests/components/__snapshots__/concurrency.test.ts.snap @@ -1164,6 +1164,69 @@ exports[`delayed rendering, but then initial rendering is cancelled by yet anoth }" `; +exports[`delayed rendering, destruction, stuff happens 1`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + const b2 = text(\`A\`); + const b3 = component(\`B\`, {value: ctx['state'].value}, key + \`__1\`, node, ctx); + return multi([b2, b3]); + } +}" +`; + +exports[`delayed rendering, destruction, stuff happens 2`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + let b2,b3; + b2 = text(\`B\`); + if (ctx['state'].hasChild) { + b3 = component(\`C\`, {value: ctx['state'].someValue+ctx['props'].value}, key + \`__1\`, node, ctx); + } + return multi([b2, b3]); + } +}" +`; + +exports[`delayed rendering, destruction, stuff happens 3`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + let block4 = createBlock(\`

\`); + + return function template(ctx, node, key = \\"\\") { + const b2 = text(\`C\`); + const b3 = component(\`D\`, {}, key + \`__1\`, node, ctx); + let txt1 = ctx['props'].value; + const b4 = block4([txt1]); + return multi([b2, b3, b4]); + } +}" +`; + +exports[`delayed rendering, destruction, stuff happens 4`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + let block3 = createBlock(\`\`); + + return function template(ctx, node, key = \\"\\") { + const b2 = text(\`D\`); + let hdlr1 = [ctx['increment'], ctx]; + let txt1 = ctx['state'].val; + const b3 = block3([hdlr1, txt1]); + return multi([b2, b3]); + } +}" +`; + exports[`delayed rendering, reusing fiber and stuff 1`] = ` "function anonymous(bdom, helpers ) { diff --git a/tests/components/concurrency.test.ts b/tests/components/concurrency.test.ts index b4526933..f4e40bfa 100644 --- a/tests/components/concurrency.test.ts +++ b/tests/components/concurrency.test.ts @@ -3783,6 +3783,106 @@ test("destroyed component causes other soon to be destroyed component to rerende expect(fixture.innerHTML).toBe(" A 22"); }); +test("delayed rendering, destruction, stuff happens", async () => { + const promC = makeDeferred(); + let stateB: any = null; + + class D extends Component { + static template = xml`D`; + state = useState({ val: 1 }); + setup() { + useLogLifecycle(); + } + increment() { + this.state.val++; + } + } + + class C extends Component { + static template = xml`C

`; + static components = { D }; + setup() { + useLogLifecycle(); + onWillUpdateProps(() => promC); + } + } + + class B extends Component { + static template = xml`B`; + static components = { C }; + state = useState({ someValue: 3, hasChild: true }); + setup() { + useLogLifecycle(); + stateB = this.state; + } + } + + class A extends Component { + static template = xml`A`; + static components = { B }; + state = useState({ value: 33 }); + setup() { + useLogLifecycle(); + } + } + + const parent = await mount(A, fixture); + expect(fixture.innerHTML).toBe("ABCD

36

"); + expect([ + "A:setup", + "A:willStart", + "A:willRender", + "B:setup", + "B:willStart", + "A:rendered", + "B:willRender", + "C:setup", + "C:willStart", + "B:rendered", + "C:willRender", + "D:setup", + "D:willStart", + "C:rendered", + "D:willRender", + "D:rendered", + "D:mounted", + "C:mounted", + "B:mounted", + "A:mounted", + ]).toBeLogged(); + + // render in A, it updates B and C, but render is blocked in C + parent.state.value = 50; + await nextTick(); + expect([ + "A:willRender", + "B:willUpdateProps", + "A:rendered", + "B:willRender", + "C:willUpdateProps", + "B:rendered", + ]).toBeLogged(); + + // update B => removes child C + stateB.hasChild = false; + // update D => render should be delayed, because AB is currently rendering + fixture.querySelector("button")!.click(); + await nextTick(); + expect([ + "B:willRender", + "B:rendered", + "A:willPatch", + "B:willPatch", + "C:willUnmount", + "D:willUnmount", + "D:willDestroy", + "C:willDestroy", + "B:patched", + "A:patched", + ]).toBeLogged(); + expect(fixture.innerHTML).toBe("AB"); +}); + // test.skip("components with shouldUpdate=false", async () => { // const state = { p: 1, cc: 10 };