[IMP] hooks: add onWillPatch and onPatched

This commit also changes the behaviour of willPatch and patched: they no
longer transfer data from one to the other, because it can be done
cleanly by hooks and a closure.

closes #307

doc/component.md

@@ -329,9 +329,6 @@ before an actual DOM patch, and is only intended to be used to save some local
 DOM state. Also, it will not be called if the component is not in the DOM (this can
 happen with components with `t-keepalive`).
 
-The return value of this method will be given as the first argument of the
-corresponding `patched` call.
-
 #### `patched(snapshot)`
 
 This hook is called whenever a component did actually update its DOM (most
@@ -342,8 +339,6 @@ with the DOM (for example, through an external library) whenever the
 component was patched. Note that this hook will not be called if the compoent is
 not in the DOM (this can happen with components with `t-keepalive`).
 
-The `snapshot` parameter is the result of the previous `willPatch` call.
-
 Updating the compoent state in this hook is possible, but not encouraged.
 One need to be careful, because updates here will cause rerender, which in
 turn will cause other calls to patched. So, we need to be particularly
@@ -1219,7 +1214,7 @@ class Node extends Component {
             </t>
         </g>
     `;
-    static components = { Node };
+  static components = { Node };
 }
 
 class RootNode extends Component {
@@ -1230,12 +1225,12 @@ class RootNode extends Component {
     `;
   static components = { Node };
   graph = {
-      label: "a",
+    label: "a",
-      children: [
+    children: [
-          {label: "b"},
+      { label: "b" },
-          {label: "c", children: [{label: "d"}, {label: "e"}]},
+      { label: "c", children: [{ label: "d" }, { label: "e" }] },
-          {label: "f", children: [{label: "g"}]},
+      { label: "f", children: [{ label: "g" }] }
-      ]
+    ]
   };
 }
 ```
@@ -1247,4 +1242,4 @@ here: the `Node` component uses itself as a subcomponent.
 Note that since SVG needs to be handled in a specific way (its namespace needs
 to be properly set), there is a small constraint for Owl components: if an owl
 component is supposed to be a part of an svg graph, then its root node needs to
-be a `g` tag, so Owl can properly set the namespace.
+be a `g` tag, so Owl can properly set the namespace.

doc/hooks.md

@@ -3,7 +3,8 @@
 ## Content
 
 - [Overview](#overview)
-- [Example](#example)
+- [Example: Mouse Position](#example-mouse-position)
+- [Example: Autofocus](#example-autofocus)
 - [Reference](#reference)
   - [One Rule](#one-rule)
   - [`useState`](#usestate)
@@ -28,7 +29,7 @@ Hooks works beautifully with Owl components: they solve the problems mentioned
 above, and in particular, they are the perfect way to make your component
 reactive.
 
-## Example
+## Example: mouse position
 
 Here is the classical example of a non trivial hook to track the mouse position.
 
@@ -57,9 +58,9 @@ function useMouse() {
 // Main root component
 class App extends owl.Component {
   static template = xml`
-      <div t-name="App">
+    <div t-name="App">
-        <div>Mouse: <t t-esc="mouse.x"/>, <t t-esc="mouse.y"/></div>
+      <div>Mouse: <t t-esc="mouse.x"/>, <t t-esc="mouse.y"/></div>
-      </div>`;
+    </div>`;
 
   // this hooks is bound to the 'mouse' property.
   mouse = useMouse();
@@ -69,6 +70,49 @@ class App extends owl.Component {
 Note that we use the prefix `use` for hooks, just like in React. This is just
 a convention.
 
+## Example: autofocus
+
+Hooks can be combined to create the desired effect. For example, the following
+hook combines the `useRef` hook with the `onPatched` and `onMounted` functions
+to create an easy way to focus an input whenever it appears in the DOM:
+
+```js
+function useAutofocus(name) {
+  let ref = useRef(name);
+  let isInDom = false;
+  function updateFocus() {
+    if (!isInDom && ref.el) {
+      isInDom = true;
+      ref.el.focus();
+    } else if (isInDom && !ref.el) {
+      isInDom = false;
+    }
+  }
+  onPatched(updateFocus);
+  onMounted(updateFocus);
+}
+```
+
+This hook takes the name of a valid `t-ref` directive, which should be present
+in the template. It then checks whenever the component is mounted or patched if
+the reference is not valid, and in this case, it will focus the node element.
+This hook can be used like this:
+
+```js
+class SomeComponent extends Component {
+  static template = xml`
+    <div>
+        <input />
+        <input t-ref="myinput"/>
+    </div>`;
+
+  constructor(...args) {
+    super(...args);
+    useAutofocus("myinput");
+  }
+}
+```
+
 ## Reference
 
 ### One rule
@@ -79,21 +123,21 @@ constructor (or in class fields):
 ```js
 // ok
 class SomeComponent extends Component {
-  state = useState({value: 0});
+  state = useState({ value: 0 });
 }
 
 // also ok
 class SomeComponent extends Component {
   constructor(...args) {
     super(...args);
-    this.state = useState({value: 0});
+    this.state = useState({ value: 0 });
   }
 }
 
 // not ok: this is executed after the constructor is called
 class SomeComponent extends Component {
   async willStart() {
-    this.state = useState({value: 0});
+    this.state = useState({ value: 0 });
   }
 }
 ```

src/component/component.ts

@@ -49,7 +49,6 @@ export interface Fiber<Props> {
   patchQueue: Fiber<any>[];
   component: Component<any, any>;
   vnode: VNode | null;
-  willPatchResult: any;
   props: Props;
   promise: Promise<VNode> | null;
   //   handlers?: any;
@@ -85,6 +84,8 @@ interface Internal<T extends Env, Props> {
   render: CompiledTemplate | null;
   mountedHandlers: { [key: number]: Function };
   willUnmountCB: Function | null;
+  willPatchCB: Function | null;
+  patchedCB: Function | null;
   classObj: { [key: string]: boolean } | null;
   refs: { [key: string]: Component<T, any> | HTMLElement | undefined } | null;
 }
@@ -187,6 +188,8 @@ export class Component<T extends Env, Props extends {}> {
       boundHandlers: {},
       mountedHandlers: {},
       willUnmountCB: null,
+      willPatchCB: null,
+      patchedCB: null,
       observer: null,
       render: null,
       classObj: null,
@@ -237,8 +240,6 @@ export class Component<T extends Env, Props extends {}> {
    * It is not called on the initial render.  This is useful to get some
    * information which are in the DOM.  For example, the current position of the
    * scrollbar
-   *
-   * The return value of willPatch will be given to the patched function.
    */
   willPatch(): any {}
 
@@ -254,10 +255,8 @@ export class Component<T extends Env, Props extends {}> {
    * One need to be careful, because updates here will cause rerender, which in
    * turn will cause other calls to updated. So, we need to be particularly
    * careful at avoiding endless cycles.
-   *
-   * The snapshot parameter is the result of the call to willPatch.
    */
-  patched(snapshot: any) {}
+  patched() {}
 
   /**
    * willUnmount is a hook that is called each time just before a component is
@@ -362,7 +361,6 @@ export class Component<T extends Env, Props extends {}> {
       component: this,
       vnode: null,
       patchQueue: parent ? parent.patchQueue : [],
-      willPatchResult: null,
       props: this.props,
       promise: null
     };
@@ -686,9 +684,11 @@ export class Component<T extends Env, Props extends {}> {
     try {
       const patchLen = patchQueue.length;
       for (let i = 0; i < patchLen; i++) {
-        const fiber = patchQueue[i];
+        component = patchQueue[i].component;
-        component = fiber.component;
+        if (component.__owl__.willPatchCB) {
-        fiber.willPatchResult = component.willPatch();
+          component.__owl__.willPatchCB();
+        }
+        component.willPatch();
       }
       for (let i = 0; i < patchLen; i++) {
         const fiber = patchQueue[i];
@@ -696,9 +696,11 @@ export class Component<T extends Env, Props extends {}> {
         component.__patch(fiber.vnode);
       }
       for (let i = patchLen - 1; i >= 0; i--) {
-        const fiber = patchQueue[i];
+        component = patchQueue[i].component;
-        component = fiber.component;
+        component.patched();
-        component.patched(fiber.willPatchResult);
+        if (component.__owl__.patchedCB) {
+          component.__owl__.patchedCB();
+        }
       }
     } catch (e) {
       errorHandler(e, component);

src/hooks.ts

@@ -40,23 +40,35 @@ export function onMounted(cb) {
   component.__owl__.mountedHandlers[`h${nextID++}`] = cb;
 }
 
+function makeLifecycleHook(method: string, reverse: boolean = false) {
+  return function(cb) {
+    const component: Component<any, any> = Component._current;
+    if (component.__owl__[method]) {
+      const current = component.__owl__[method];
+      if (reverse) {
+        component.__owl__[method] = function() {
+          current.call(component);
+          cb.call(component);
+        };
+      } else {
+        component.__owl__[method] = function() {
+          cb.call(component);
+          current.call(component);
+        };
+      }
+    } else {
+      component.__owl__[method] = cb;
+    }
+  };
+}
+
 /**
  * willUnmount hook. The callback will be called when the current component is
  * willUnmounted.  Note that the component mounted method is called last.
  */
-export function onWillUnmount(cb) {
+export const onWillUnmount = makeLifecycleHook("willUnmountCB");
-  const component: Component<any, any> = Component._current;
+export const onWillPatch = makeLifecycleHook("willPatchCB");
-  if (component.__owl__.willUnmountCB) {
+export const onPatched = makeLifecycleHook("patchedCB", true);
-    const current = component.__owl__.willUnmountCB;
-    component.__owl__.willUnmountCB = function() {
-      cb.call(component);
-      current.call(component);
-    };
-  } else {
-    component.__owl__.willUnmountCB = cb;
-  }
-}
-
 /**
  * useRef hook
  *

tests/component/component.test.ts

@@ -725,10 +725,8 @@ describe("lifecycle hooks", () => {
       state = useState({ n: 1 });
       willPatch() {
         steps.push("parent:willPatch");
-        return "leffe";
       }
-      patched(snapshot) {
+      patched() {
-        expect(snapshot).toBe("leffe");
         steps.push("parent:patched");
       }
     }

tests/hooks.test.ts

@@ -1,6 +1,6 @@
 import { makeTestEnv, makeTestFixture, nextTick } from "./helpers";
 import { Component, Env } from "../src/component/component";
-import { useState, onMounted, onWillUnmount, useRef } from "../src/hooks";
+import { useState, onMounted, onWillUnmount, useRef, onPatched, onWillPatch } from "../src/hooks";
 import { xml } from "../src/tags";
 
 //------------------------------------------------------------------------------
@@ -149,4 +149,171 @@ describe("hooks", () => {
     await nextTick();
     expect(fixture.innerHTML).toBe("<div><button>1</button></div>");
   });
+
+  test("can use onPatched, onWillPatch", async () => {
+    const steps: string[] = [];
+    function useMyHook() {
+      onWillPatch(() => {
+        steps.push("willPatch");
+      });
+      onPatched(() => {
+        steps.push("patched");
+      });
+    }
+
+    class MyComponent extends Component<any, any> {
+      static template = xml`<div><t t-if="state.flag">hey</t></div>`;
+      state = useState({ flag: true });
+
+      constructor(env) {
+        super(env);
+        useMyHook();
+      }
+    }
+
+    const component = new MyComponent(env);
+    await component.mount(fixture);
+    expect(component).not.toHaveProperty("patched");
+    expect(component).not.toHaveProperty("willPatch");
+    expect(steps).toEqual([]);
+
+    expect(fixture.innerHTML).toBe("<div>hey</div>");
+    component.state.flag = false;
+    await nextTick();
+    expect(fixture.innerHTML).toBe("<div></div>");
+
+    expect(steps).toEqual(["willPatch", "patched"]);
+  });
+
+  test("patched, willPatch, onPatched, onWillPatch order", async () => {
+    const steps: string[] = [];
+    function useMyHook() {
+      onPatched(() => {
+        steps.push("hook:patched");
+      });
+      onWillPatch(() => {
+        steps.push("hook:willPatch");
+      });
+    }
+    class MyComponent extends Component<any, any> {
+      static template = xml`<div><t t-if="state.flag">hey</t></div>`;
+      state = useState({ flag: true });
+
+      constructor(env) {
+        super(env);
+        useMyHook();
+      }
+      willPatch() {
+        steps.push("comp:willPatch");
+      }
+      patched() {
+        steps.push("comp:patched");
+      }
+    }
+    const component = new MyComponent(env);
+    await component.mount(fixture);
+    expect(fixture.innerHTML).toBe("<div>hey</div>");
+    component.state.flag = false;
+    await nextTick();
+
+    expect(steps).toEqual(["hook:willPatch", "comp:willPatch", "comp:patched", "hook:patched"]);
+  });
+
+  test("two different call to willPatch/patched should work", async () => {
+    const steps: string[] = [];
+    function useMyHook(i) {
+      onPatched(() => {
+        steps.push("hook:patched" + i);
+      });
+      onWillPatch(() => {
+        steps.push("hook:willPatch" + i);
+      });
+    }
+    class MyComponent extends Component<any, any> {
+      static template = xml`<div>hey<t t-esc="state.value"/></div>`;
+      state = useState({ value: 1 });
+      constructor(env) {
+        super(env);
+        useMyHook(1);
+        useMyHook(2);
+      }
+    }
+    const component = new MyComponent(env);
+    await component.mount(fixture);
+    expect(fixture.innerHTML).toBe("<div>hey1</div>");
+    component.state.value++;
+    await nextTick();
+    expect(fixture.innerHTML).toBe("<div>hey2</div>");
+
+    expect(steps).toEqual([
+      "hook:willPatch2",
+      "hook:willPatch1",
+      "hook:patched1",
+      "hook:patched2"
+    ]);
+  });
+
+  describe("autofocus hook", () => {
+    function useAutofocus(name) {
+      let ref = useRef(name);
+      let isInDom = false;
+      function updateFocus() {
+        if (!isInDom && ref.el) {
+          isInDom = true;
+          ref.el.focus();
+        } else if (isInDom && !ref.el) {
+          isInDom = false;
+        }
+      }
+      onPatched(updateFocus);
+      onMounted(updateFocus);
+    }
+
+    test("simple input", async () => {
+      class SomeComponent extends Component<any, any> {
+        static template = xml`
+            <div>
+                <input t-ref="input1"/>
+                <input t-ref="input2"/>
+            </div>`;
+
+        constructor(env) {
+          super(env);
+          useAutofocus("input2");
+        }
+      }
+
+      const component = new SomeComponent(env);
+      await component.mount(fixture);
+      expect(fixture.innerHTML).toBe("<div><input><input></div>");
+      const input2 = fixture.querySelectorAll("input")[1];
+      expect(input2).toBe(document.activeElement);
+    });
+
+    test("input in a t-if", async () => {
+      class SomeComponent extends Component<any, any> {
+        static template = xml`
+            <div>
+                <input t-ref="input1"/>
+                <t t-if="state.flag"><input t-ref="input2"/></t>
+            </div>`;
+
+        state = useState({ flag: false });
+        constructor(env) {
+          super(env);
+          useAutofocus("input2");
+        }
+      }
+
+      const component = new SomeComponent(env);
+      await component.mount(fixture);
+      expect(fixture.innerHTML).toBe("<div><input></div>");
+      expect(document.activeElement).toBe(document.body);
+
+      component.state.flag = true;
+      await nextTick();
+      const input2 = fixture.querySelectorAll("input")[1];
+      expect(input2).toBe(document.activeElement);
+    });
+  });
 });