From bd5637c0a3db4abf435d2d1215b1edb20f0090ff Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= <ged@odoo.com>
Date: Tue, 21 Dec 2021 16:36:36 +0100
Subject: [PATCH] [FIX] useEffect: can depend on dom dependencies

Because the dependencies are now computed in patched.
---
 src/hooks.ts                                  | 17 ++++-----
 .../__snapshots__/hooks.test.ts.snap          | 19 ++++++++++
 tests/components/hooks.test.ts                | 35 +++++++++++++++++--
 3 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/src/hooks.ts b/src/hooks.ts
index c55b7dfc..21db0d1d 100644
--- a/src/hooks.ts
+++ b/src/hooks.ts
@@ -1,6 +1,6 @@
 import type { Env } from "./app/app";
 import { getCurrent } from "./component/component_node";
-import { onMounted, onPatched, onWillPatch, onWillUnmount } from "./component/lifecycle_hooks";
+import { onMounted, onPatched, onWillUnmount } from "./component/lifecycle_hooks";
 
 // -----------------------------------------------------------------------------
 // useRef
@@ -75,17 +75,12 @@ export function useEffect(effect: Effect, computeDependencies: () => any[] = ()
     cleanup = effect(...dependencies) || NO_OP;
   });
 
-  let shouldReapplyOnPatch = false;
-  onWillPatch(() => {
-    const newDeps = computeDependencies();
-    shouldReapplyOnPatch = newDeps.some((val, i) => val !== dependencies[i]);
-    if (shouldReapplyOnPatch) {
-      cleanup();
-      dependencies = newDeps;
-    }
-  });
   onPatched(() => {
-    if (shouldReapplyOnPatch) {
+    const newDeps = computeDependencies();
+    const shouldReapply = newDeps.some((val, i) => val !== dependencies[i]);
+    if (shouldReapply) {
+      dependencies = newDeps;
+      cleanup();
       cleanup = effect(...dependencies) || NO_OP;
     }
   });
diff --git a/tests/components/__snapshots__/hooks.test.ts.snap b/tests/components/__snapshots__/hooks.test.ts.snap
index fd988e2a..082c1a14 100644
--- a/tests/components/__snapshots__/hooks.test.ts.snap
+++ b/tests/components/__snapshots__/hooks.test.ts.snap
@@ -197,6 +197,25 @@ exports[`hooks useEffect hook dependencies prevent effects from rerunning when u
 }"
 `;
 
+exports[`hooks useEffect hook effect can depend on stuff in dom 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  let block2 = createBlock(\`<div block-ref=\\"0\\"/>\`);
+  
+  return function template(ctx, node, key = \\"\\") {
+    const refs = ctx.__owl__.refs;
+    let b2;
+    if (ctx['state'].value) {
+      let d1 = (el) => refs[\`div\`] = el;
+      b2 = block2([d1]);
+    }
+    return multi([b2]);
+  }
+}"
+`;
+
 exports[`hooks useEffect hook effect runs on mount, is reapplied on patch, and is cleaned up on unmount and before reapplying 1`] = `
 "function anonymous(bdom, helpers
 ) {
diff --git a/tests/components/hooks.test.ts b/tests/components/hooks.test.ts
index cc15d92a..a7df5584 100644
--- a/tests/components/hooks.test.ts
+++ b/tests/components/hooks.test.ts
@@ -17,7 +17,7 @@ import {
   useSubEnv,
   xml,
 } from "../../src/index";
-import { elem, makeTestFixture, nextTick, snapshotEverything } from "../helpers";
+import { elem, logStep, makeTestFixture, nextTick, snapshotEverything } from "../helpers";
 
 let fixture: HTMLElement;
 
@@ -396,6 +396,35 @@ describe("hooks", () => {
       ]);
     });
 
+    test("effect can depend on stuff in dom", async () => {
+      class MyComponent extends Component {
+        static template = xml`
+          <t t-if="state.value">
+            <div t-ref="div"/>
+          </t>`;
+        state = useState({
+          value: false,
+        });
+        setup() {
+          const ref = useRef("div");
+          useEffect(
+            (el) => {
+              logStep("effect started:" + (el ? "EL" : "NULL"));
+              return () => logStep("cleaning up effect:" + (el ? "EL" : "NULL"));
+            },
+            () => [ref.el]
+          );
+        }
+      }
+      const component = await mount(MyComponent, fixture);
+
+      expect(["effect started:NULL"]).toBeLogged();
+
+      component.state.value = true;
+      await nextTick();
+      expect(["cleaning up effect:NULL", "effect started:EL"]).toBeLogged();
+    });
+
     test("dependencies prevent effects from rerunning when unchanged", async () => {
       let steps = [];
       class MyComponent extends Component {
@@ -455,22 +484,22 @@ describe("hooks", () => {
 
         "before state mutation: a",
         // Cleanups run in reverse order
-        "cleaning up for ab: {a: 0, b: 0}",
         // Cleanup for b is not run
         "cleaning up for a: 0",
 
         "Effect a: 1",
+        "cleaning up for ab: {a: 0, b: 0}",
         // Effect b is not run
         "Effect ab: {a: 1, b: 0}",
         "after state mutation: a",
 
         "before state mutation: b",
-        "cleaning up for ab: {a: 1, b: 0}",
         "cleaning up for b: 0",
         // Cleanup for a is not run
 
         // Effect a is not run
         "Effect b: 1",
+        "cleaning up for ab: {a: 1, b: 0}",
         "Effect ab: {a: 1, b: 1}",
         "after state mutation: b",