[FIX] useEffect: properly handle errors in effect function

Before this commit, if the effect function would throw, then the cleanup
function would not be properly assigned, which caused additional errors
later, when the cleanup code would try to call it.

closes #1149

src/hooks.ts

@@ -57,7 +57,6 @@ export function useChildSubEnv(envExtension: Env) {
 // useEffect
 // -----------------------------------------------------------------------------
 
-const NO_OP = () => {};
 /**
  * @param {...any} dependencies the dependencies computed by computeDependencies
  * @returns {void|(()=>void)} a cleanup function that reverses the side
@@ -78,11 +77,11 @@ type Effect = (...dependencies: any[]) => void | (() => void);
  *      NaN !== NaN, which will cause the effect to rerun on every patch.
  */
 export function useEffect(effect: Effect, computeDependencies: () => any[] = () => [NaN]) {
-  let cleanup: () => void;
+  let cleanup: (() => void) | void;
   let dependencies: any[];
   onMounted(() => {
     dependencies = computeDependencies();
-    cleanup = effect(...dependencies) || NO_OP;
+    cleanup = effect(...dependencies);
   });
 
   onPatched(() => {
@@ -90,12 +89,14 @@ export function useEffect(effect: Effect, computeDependencies: () => any[] = ()
     const shouldReapply = newDeps.some((val, i) => val !== dependencies[i]);
     if (shouldReapply) {
       dependencies = newDeps;
-      cleanup();
-      cleanup = effect(...dependencies) || NO_OP;
+      if (cleanup) {
+        cleanup();
+      }
+      cleanup = effect(...dependencies);
     }
   });
 
-  onWillUnmount(() => cleanup());
+  onWillUnmount(() => cleanup && cleanup());
 }
 
 // -----------------------------------------------------------------------------

tests/components/__snapshots__/hooks.test.ts.snap

@@ -301,6 +301,19 @@ exports[`hooks useEffect hook effect with empty dependency list never reruns 1`]
 }"
 `;
 
+exports[`hooks useEffect hook properly behaves when the effect function throws 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  let block1 = createBlock(\`<div/>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    return block1();
+  }
+}"
+`;
+
 exports[`hooks useExternalListener 1`] = `
 "function anonymous(bdom, helpers
 ) {

tests/components/hooks.test.ts

@@ -632,5 +632,35 @@ describe("hooks", () => {
         "cleaning up for 1",
       ]);
     });
+
+    test("properly behaves when the effect function throws", async () => {
+      let originalconsoleError = console.error;
+      let originalconsoleWarn = console.warn;
+      console.error = jest.fn(() => {});
+      console.warn = jest.fn(() => {});
+      class MyComponent extends Component {
+        static template = xml`<div/>`;
+        setup() {
+          useEffect(
+            () => {
+              throw new Error("Intentional error");
+            },
+            () => []
+          );
+        }
+      }
+
+      try {
+        await mount(MyComponent, fixture);
+      } catch (e: any) {
+        expect(e.message).toBe("Intentional error");
+      }
+      // no console.error because the error has been caught in this test
+      expect(console.error).toHaveBeenCalledTimes(0);
+      console.error = originalconsoleError;
+      // 1 console.warn because app is destroyed
+      expect(console.warn).toHaveBeenCalledTimes(1);
+      console.warn = originalconsoleWarn;
+    });
   });
 });