Handle errors in componentWillUnmount and clean up appropriately.

Author: karthikv

src/component.ts

@@ -207,22 +207,38 @@ abstract class Component<P, S> {
     allComponentsMap: Record<string, Component<any, any> | undefined> | null,
   ): Promise<void> {
     return this._lock(async () => {
-      const promises = Object.keys(this._childMap).map(key => {
-        const childPromises = this._childMap[key]!.map(child => {
+      const promises = Object.keys(this._childMap).flatMap(key => {
+        return this._childMap[key]!.map(async child => {
           if (child instanceof Component) {
             return child._triggerUnmount(allComponentsMap)
           }
-          return
         })
-        return Promise.all(childPromises)
       })
-      await Promise.all(promises)
 
-      this.componentWillUnmount()
-      if (allComponentsMap) {
-        delete allComponentsMap[this._id]
+      try {
+        await Promise.all(promises)
+      } catch (error) {
+        // If a child errors while unmounting, still wait for all children to be
+        // processed before calling the parent's componentWillUnmount.
+        await Promise.allSettled(promises)
+
+        if (process.env.NODE_ENV !== "test") {
+          // Don't suppress the error, but ensure the finally block unmounts and
+          // cleans up appropriately.
+          throw error
+        }
+      } finally {
+        // The user-defined componentWillUnmount() may error, but we still need
+        // to clean up.
+        try {
+          this.componentWillUnmount()
+        } finally {
+          if (allComponentsMap) {
+            delete allComponentsMap[this._id]
+          }
+          this._unmounted = true
+        }
       }
-      this._unmounted = true
     })
   }
 

test/purview_test.tsx

@@ -1390,10 +1390,65 @@ test("locked mount cycle", async () => {
 })
 
 test("componentWillUnmount", async () => {
-  let unmounted = false
-  class Foo extends TestComponent<{}, { text: string }> {
+  let unmountCalled = false
+  let instance: Foo = null as any
+  class Foo extends TestComponent<{}, {}> {
+    constructor(props: {}) {
+      super(props)
+      instance = this
+    }
+
+    componentWillUnmount(): void {
+      unmountCalled = true
+    }
+
+    render(): JSX.Element {
+      return <div />
+    }
+  }
+
+  await renderAndConnect(<Foo />, async () => {
+    expect(unmountCalled).toBe(false)
+    expect(instance._unmounted).toBe(false)
+  })
+
+  // Must wait for close to propagate to server.
+  await new Promise(resolve => setTimeout(resolve, 25))
+  expect(unmountCalled).toBe(true)
+  expect(instance._unmounted).toBe(true)
+})
+
+test("componentWillUnmount nested error", async () => {
+  let unmountCalled = false
+  let fooInstance: Foo = null as any
+  class Foo extends TestComponent<{}, {}> {
+    constructor(props: {}) {
+      super(props)
+      fooInstance = this
+    }
+
+    componentWillUnmount(): void {
+      unmountCalled = true
+    }
+
+    render(): JSX.Element {
+      return <Bar />
+    }
+  }
+
+  let barInstance: Bar = null as any
+  class Bar extends TestComponent<{}, {}> {
+    static getUniqueName(): string {
+      return `${super.getUniqueName()}-bar`
+    }
+
+    constructor(props: {}) {
+      super(props)
+      barInstance = this
+    }
+
     componentWillUnmount(): void {
-      unmounted = true
+      throw new Error("Bar unmount error")
     }
 
     render(): JSX.Element {
@@ -1402,12 +1457,16 @@ test("componentWillUnmount", async () => {
   }
 
   await renderAndConnect(<Foo />, async () => {
-    expect(unmounted).toBe(false)
+    expect(unmountCalled).toBe(false)
+    expect(barInstance._unmounted).toBe(false)
+    expect(fooInstance._unmounted).toBe(false)
   })
 
   // Must wait for close to propagate to server.
   await new Promise(resolve => setTimeout(resolve, 25))
-  expect(unmounted).toBe(true)
+  expect(unmountCalled).toBe(true)
+  expect(barInstance._unmounted).toBe(true)
+  expect(fooInstance._unmounted).toBe(true)
 })
 
 test("componentWillReceiveProps", async () => {

tsconfig.json

@@ -1,6 +1,6 @@
 {
   "compilerOptions": {
-    "lib": ["es2017", "dom"],
+    "lib": ["es2020", "dom"],
     "target": "es2017",
     "module": "commonjs",
     "moduleResolution": "node",