From fddb1ec924c5c3a34a10972e71697bbb0abcdd1f Mon Sep 17 00:00:00 2001 From: Samuel Degueldre Date: Thu, 11 Apr 2024 15:26:14 +0200 Subject: [PATCH] [FIX] runtime: don't emit async hook warnings when cancelled/destroyed Previously, the async hook warning that's emitted when async hooks take too long to resolve would be emitted even if the component was destroyed or cancelled. We used to check if the node's fiber was still the current fiber as a proxy for cancellation, but this is insufficient in the case where an app might be destroyed. This commit also wraps the content of some of the warning tests into a try/finally block so that a failure of these tests doesn't pollute setTimeout and console.warn and cause other tests to timeout one after another. --- src/runtime/lifecycle_hooks.ts | 2 +- .../__snapshots__/lifecycle.test.ts.snap | 13 ++ tests/components/lifecycle.test.ts | 115 ++++++++++++------ 3 files changed, 94 insertions(+), 36 deletions(-) diff --git a/src/runtime/lifecycle_hooks.ts b/src/runtime/lifecycle_hooks.ts index 0c98cbe8..99290086 100644 --- a/src/runtime/lifecycle_hooks.ts +++ b/src/runtime/lifecycle_hooks.ts @@ -28,7 +28,7 @@ function wrapError(fn: (...args: any[]) => any, hookName: string) { result.catch(() => {}), new Promise((resolve) => setTimeout(() => resolve(TIMEOUT), 3000)), ]).then((res) => { - if (res === TIMEOUT && node.fiber === fiber) { + if (res === TIMEOUT && node.fiber === fiber && node.status <= 2) { console.warn(timeoutError); } }); diff --git a/tests/components/__snapshots__/lifecycle.test.ts.snap b/tests/components/__snapshots__/lifecycle.test.ts.snap index 6d8cc811..8b5bce58 100644 --- a/tests/components/__snapshots__/lifecycle.test.ts.snap +++ b/tests/components/__snapshots__/lifecycle.test.ts.snap @@ -683,6 +683,19 @@ exports[`lifecycle hooks sub widget (inside sub node): hooks are correctly calle }" `; +exports[`lifecycle hooks timeout in onWillStart doesn't emit a warning if app is destroyed 1`] = ` +"function anonymous(app, bdom, helpers +) { + let { text, createBlock, list, multi, html, toggler, comment } = bdom; + + let block1 = createBlock(\`\`); + + return function template(ctx, node, key = \\"\\") { + return block1(); + } +}" +`; + exports[`lifecycle hooks timeout in onWillStart emits a warning 1`] = ` "function anonymous(app, bdom, helpers ) { diff --git a/tests/components/lifecycle.test.ts b/tests/components/lifecycle.test.ts index a140b2b4..d223f54e 100644 --- a/tests/components/lifecycle.test.ts +++ b/tests/components/lifecycle.test.ts @@ -1,5 +1,9 @@ -import { App, Component, mount, onMounted, onWillStart, useState, xml } from "../../src"; import { + App, + Component, + mount, + useState, + xml, onWillPatch, onWillUnmount, onPatched, @@ -7,7 +11,9 @@ import { onWillRender, onWillDestroy, onRendered, -} from "../../src/runtime/lifecycle_hooks"; + onMounted, + onWillStart, +} from "../../src"; import { status } from "../../src/runtime/status"; import { elem, @@ -117,24 +123,60 @@ describe("lifecycle hooks", () => { timeoutCbs[++timeoutId] = cb; return timeoutId; }) as any; - class Test extends Component { - static template = xml``; - setup() { - onWillStart(() => new Promise(() => {})); + try { + class Test extends Component { + static template = xml``; + setup() { + onWillStart(() => new Promise(() => {})); + } } + mount(Test, fixture, { test: true }); + nextTick(); + for (const id in timeoutCbs) { + timeoutCbs[id](); + delete timeoutCbs[id]; + } + await nextMicroTick(); + await nextMicroTick(); + expect(console.warn).toHaveBeenCalledTimes(1); + expect(warnArgs![0]!.message).toBe("onWillStart's promise hasn't resolved after 3 seconds"); + } finally { + console.warn = warn; + window.setTimeout = setTimeout; } - mount(Test, fixture, { test: true }); - nextTick(); - for (const id in timeoutCbs) { - timeoutCbs[id](); - delete timeoutCbs[id]; + }); + + test("timeout in onWillStart doesn't emit a warning if app is destroyed", async () => { + const { warn } = console; + console.warn = jest.fn(); + const { setTimeout } = window; + let timeoutCbs: any = {}; + let timeoutId = 0; + window.setTimeout = ((cb: any) => { + timeoutCbs[++timeoutId] = cb; + return timeoutId; + }) as any; + try { + class Test extends Component { + static template = xml``; + setup() { + onWillStart(() => new Promise(() => {})); + } + } + const app = new App(Test, { test: true }); + app.mount(fixture); + app.destroy(); + for (const id in timeoutCbs) { + timeoutCbs[id](); + delete timeoutCbs[id]; + } + await nextMicroTick(); + await nextMicroTick(); + expect(console.warn).toHaveBeenCalledTimes(0); + } finally { + console.warn = warn; + window.setTimeout = setTimeout; } - await nextMicroTick(); - await nextMicroTick(); - expect(console.warn).toHaveBeenCalledTimes(1); - expect(warnArgs![0]!.message).toBe("onWillStart's promise hasn't resolved after 3 seconds"); - console.warn = warn; - window.setTimeout = setTimeout; }); test("timeout in onWillUpdateProps emits a warning", async () => { @@ -162,25 +204,28 @@ describe("lifecycle hooks", () => { return timeoutId; }) as any; - parent.state.prop = 2; - let tick = nextTick(); - for (const id in timeoutCbs) { - timeoutCbs[id](); - delete timeoutCbs[id]; + try { + parent.state.prop = 2; + let tick = nextTick(); + for (const id in timeoutCbs) { + timeoutCbs[id](); + delete timeoutCbs[id]; + } + await tick; + tick = nextTick(); + for (const id in timeoutCbs) { + timeoutCbs[id](); + delete timeoutCbs[id]; + } + await tick; + expect(console.warn).toHaveBeenCalledTimes(1); + expect(warnArgs![0]!.message).toBe( + "onWillUpdateProps's promise hasn't resolved after 3 seconds" + ); + } finally { + console.warn = warn; + window.setTimeout = setTimeout; } - await tick; - tick = nextTick(); - for (const id in timeoutCbs) { - timeoutCbs[id](); - delete timeoutCbs[id]; - } - await tick; - expect(console.warn).toHaveBeenCalledTimes(1); - expect(warnArgs![0]!.message).toBe( - "onWillUpdateProps's promise hasn't resolved after 3 seconds" - ); - console.warn = warn; - window.setTimeout = setTimeout; }); test("mounted hook is called if mounted in DOM", async () => {