From b3c6ec2b4813427645996937fbb9a591459200e2 Mon Sep 17 00:00:00 2001 From: "Lucas Perais (lpe)" Date: Sat, 4 Dec 2021 18:35:31 +0100 Subject: [PATCH] [FIX] component: render in delayed willUpdateProps Have a child component on which a render is triggered. This component delays its willUpdateProps and makes a rendering during the willUpdateProps Before this commit, renderings of the child were inconsistent across its parent's renderings. After this commit, it works as expected. --- src/component/component_node.ts | 15 +- src/component/fibers.ts | 31 ++- .../__snapshots__/concurrency.test.ts.snap | 76 ++++++ tests/components/concurrency.test.ts | 220 ++++++++++++++++++ 4 files changed, 336 insertions(+), 6 deletions(-) diff --git a/src/component/component_node.ts b/src/component/component_node.ts index abb3f404..8dffc436 100644 --- a/src/component/component_node.ts +++ b/src/component/component_node.ts @@ -201,13 +201,18 @@ export class ComponentNode async updateAndRender(props: any, parentFiber: Fiber) { // update + const root = this.fiber && this.fiber.root; const fiber = makeChildFiber(this, parentFiber); + const parentRoot = parentFiber.root; + const rootChanged = root !== parentRoot; this.fiber = fiber; - if (this.willPatch.length) { - parentFiber.root.willPatch.push(fiber); - } - if (this.patched.length) { - parentFiber.root.patched.push(fiber); + if (rootChanged) { + if (this.willPatch.length) { + parentRoot.willPatch.push(fiber); + } + if (this.patched.length) { + parentRoot.patched.push(fiber); + } } const component = this.component; applyDefaultProps(props, component.constructor as any); diff --git a/src/component/fibers.ts b/src/component/fibers.ts index 7911d84e..24b20193 100644 --- a/src/component/fibers.ts +++ b/src/component/fibers.ts @@ -3,15 +3,44 @@ import type { ComponentNode } from "./component_node"; import { fibersInError, handleError } from "./error_handling"; import { STATUS } from "./status"; +/** + * Cleans on the root fiber the patch and willPatch fiber lists + * It is typically needed when the same root fiber needs to recycle on + * of its children or grandchildren's fiber. + */ +function cleanPatchableFiber(child: Fiber, root: RootFiber) { + const { willPatch, patched } = root; + let i = willPatch.indexOf(child); + if (i > -1) { + willPatch.splice(i, 1); + } + i = patched.indexOf(child); + if (i > -1) { + patched.splice(i, 1); + } +} + export function makeChildFiber(node: ComponentNode, parent: Fiber): Fiber { let current = node.fiber; if (current) { // current is necessarily a rootfiber here let root = parent.root; + const isSameRoot = current.root === root; cancelFibers(root, current.children); current.children = []; current.parent = parent; - root.counter++; + // only increment our rendering if we were not + // already accounted for, or that we have been rendered + // already (in which case our fiber was removed from the root rendering) + if (!isSameRoot || current.bdom) { + root.counter++; + } + + if (isSameRoot) { + cleanPatchableFiber(current, root); + } + + current.bdom = null; current.root = root; return current; } diff --git a/tests/components/__snapshots__/concurrency.test.ts.snap b/tests/components/__snapshots__/concurrency.test.ts.snap index 93523b13..13e75694 100644 --- a/tests/components/__snapshots__/concurrency.test.ts.snap +++ b/tests/components/__snapshots__/concurrency.test.ts.snap @@ -939,6 +939,82 @@ exports[`creating two async components, scenario 3 (patching in the same frame) }" `; +exports[`delay willUpdateProps 1`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + let b2 = text(ctx['props'].value); + let b3 = text(\`_\`); + let b4 = text(ctx['state'].int); + return multi([b2, b3, b4]); + } +}" +`; + +exports[`delay willUpdateProps 2`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + return component(\`Child\`, {value: ctx['state'].value}, key + \`__1\`, node, ctx); + } +}" +`; + +exports[`delay willUpdateProps with rendering grandchild 1`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + let b2 = text(ctx['props'].value); + let b3 = text(\`_\`); + let b4 = text(ctx['state'].int); + return multi([b2, b3, b4]); + } +}" +`; + +exports[`delay willUpdateProps with rendering grandchild 2`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + let block1 = createBlock(\`
\`); + + return function template(ctx, node, key = \\"\\") { + return block1(); + } +}" +`; + +exports[`delay willUpdateProps with rendering grandchild 3`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + let b2 = component(\`DelayedChild\`, {value: ctx['props'].state.value}, key + \`__1\`, node, ctx); + let b3 = component(\`ReactiveChild\`, {}, key + \`__2\`, node, ctx); + return multi([b2, b3]); + } +}" +`; + +exports[`delay willUpdateProps with rendering grandchild 4`] = ` +"function anonymous(bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, component, comment } = bdom; + + return function template(ctx, node, key = \\"\\") { + return component(\`Parent\`, {state: ctx['state']}, key + \`__1\`, node, ctx); + } +}" +`; + exports[`destroying/recreating a subwidget with different props (if start is not over) 1`] = ` "function anonymous(bdom, helpers ) { diff --git a/tests/components/concurrency.test.ts b/tests/components/concurrency.test.ts index b01b4818..a64832c4 100644 --- a/tests/components/concurrency.test.ts +++ b/tests/components/concurrency.test.ts @@ -2583,6 +2583,226 @@ test("parent and child rendered at exact same time", async () => { ]).toBeLogged(); }); +test("delay willUpdateProps", async () => { + let promise: any = null; + let child: any; + + class Child extends Component { + static template = xml`_`; + state: any; + setup() { + useLogLifecycle(); + child = this; + this.state = useState({ int: 0 }); + onWillUpdateProps(async () => { + await promise; + this.state.int++; + }); + } + } + + class Parent extends Component { + static template = xml``; + static components = { Child }; + state = { value: 0 }; + setup() { + useLogLifecycle(); + } + } + const parent = await mount(Parent, fixture); + expect(fixture.innerHTML).toBe("0_0"); + expect([ + "Parent:setup", + "Parent:willStart", + "Parent:willRender", + "Child:setup", + "Child:willStart", + "Parent:rendered", + "Child:willRender", + "Child:rendered", + "Child:mounted", + "Parent:mounted", + ]).toBeLogged(); + + promise = makeDeferred(); + const prom1 = promise; + parent.state.value = 1; + child.render(); // trigger a root rendering first + parent.render(); + await nextTick(); + expect(fixture.innerHTML).toBe("0_0"); + expect([ + "Child:willRender", + "Child:rendered", + "Parent:willRender", + "Child:willUpdateProps", + "Parent:rendered", + ]).toBeLogged(); + + promise = makeDeferred(); + const prom2 = promise; + parent.state.value = 2; + parent.render(); + await nextTick(); + expect(fixture.innerHTML).toBe("0_0"); + + prom2.resolve(); + await nextTick(); + expect(fixture.innerHTML).toBe("2_1"); + + expect([ + "Parent:willRender", + "Child:willUpdateProps", + "Parent:rendered", + "Child:willRender", + "Child:rendered", + "Parent:willPatch", + "Child:willPatch", + "Child:patched", + "Parent:patched", + ]).toBeLogged(); + + prom1.resolve(); + await nextTick(); + expect(fixture.innerHTML).toBe("2_2"); + + expect(["Child:willRender", "Child:rendered", "Child:willPatch", "Child:patched"]).toBeLogged(); +}); + +test("delay willUpdateProps with rendering grandchild", async () => { + // This test is a bit tricky, a Parent and one of his grandchildren render while another of the parent's + // grandchildren is awaiting its willUpdateProps. + // Technically RootFibers will be downgraded in ChildFibers, keeping the same container RootFiber. + // This case happens when Parent and ReaciveChild react together to a change in a reactive state/ + let promise: any = null; + let child: any; + let reactiveChild: any; + + // Delayed willUpdateProps + class DelayedChild extends Component { + static template = xml`_`; + state: any; + setup() { + useLogLifecycle(); + child = this; + this.state = useState({ int: 0 }); + onWillUpdateProps(async () => { + await promise; + this.state.int++; + }); + } + } + + // A sibling of the delayed component, we will trigger a render on it manually + class ReactiveChild extends Component { + static template = xml`
`; + setup() { + reactiveChild = this; + useLogLifecycle(); + } + } + + // The parent of everybody, we also manually trigger render on it. + class Parent extends Component { + static template = xml``; + static components = { DelayedChild, ReactiveChild }; + setup() { + useLogLifecycle(); + } + } + + class GrandParent extends Component { + static template = xml``; + static components = { Parent }; + state = { value: 0 }; + } + const parent = await mount(GrandParent, fixture); + expect(fixture.innerHTML).toBe("0_0
"); + expect([ + "Parent:setup", + "Parent:willStart", + "Parent:willRender", + "DelayedChild:setup", + "DelayedChild:willStart", + "ReactiveChild:setup", + "ReactiveChild:willStart", + "Parent:rendered", + "DelayedChild:willRender", + "DelayedChild:rendered", + "ReactiveChild:willRender", + "ReactiveChild:rendered", + "ReactiveChild:mounted", + "DelayedChild:mounted", + "Parent:mounted", + ]).toBeLogged(); + + promise = makeDeferred(); + const prom1 = promise; + parent.state.value = 1; + child.render(); // trigger a root rendering first + parent.render(); + reactiveChild.render(); + await nextTick(); + expect(fixture.innerHTML).toBe("0_0
"); + expect([ + "DelayedChild:willRender", + "DelayedChild:rendered", + "Parent:willUpdateProps", + "ReactiveChild:willRender", + "ReactiveChild:rendered", + "Parent:willRender", + "DelayedChild:willUpdateProps", + "ReactiveChild:willUpdateProps", + "Parent:rendered", + "ReactiveChild:willRender", + "ReactiveChild:rendered", + ]).toBeLogged(); + + promise = makeDeferred(); + const prom2 = promise; + child.render(); // trigger a root rendering first + parent.state.value = 2; + parent.render(); + reactiveChild.render(); + await nextTick(); + expect(fixture.innerHTML).toBe("0_0
"); + expect([ + "Parent:willUpdateProps", + "ReactiveChild:willRender", + "ReactiveChild:rendered", + "Parent:willRender", + "DelayedChild:willUpdateProps", + "ReactiveChild:willUpdateProps", + "Parent:rendered", + "ReactiveChild:willRender", + "ReactiveChild:rendered", + ]).toBeLogged(); + + prom2.resolve(); + await nextTick(); + expect(fixture.innerHTML).toBe("2_1
"); + expect([ + "DelayedChild:willRender", + "DelayedChild:rendered", + "Parent:willPatch", + "ReactiveChild:willPatch", + "DelayedChild:willPatch", + "DelayedChild:patched", + "ReactiveChild:patched", + "Parent:patched", + ]).toBeLogged(); + + prom1.resolve(); + await nextTick(); + expect(fixture.innerHTML).toBe("2_2
"); + expect([ + "DelayedChild:willRender", + "DelayedChild:rendered", + "DelayedChild:willPatch", + "DelayedChild:patched", + ]).toBeLogged(); +}); + // test.skip("components with shouldUpdate=false", async () => { // const state = { p: 1, cc: 10 };