add doc

also, improves the implementation of subscriptions

README.md

@@ -74,6 +74,7 @@ Are you new to Owl? This is the place to start!
 - [Tutorial: create a TodoList application](doc/learning/tutorial_todoapp.md)
 - [How to start an Owl project](doc/learning/quick_start.md)
 - [How to test Components](doc/learning/how_to_test.md)
+- [How to organize Owl code](doc/miscellaneous/organizing_code.md)
 
 ### Reference
 
@@ -82,6 +83,7 @@ Are you new to Owl? This is the place to start!
 - [Component](doc/reference/component.md)
 - [Component Lifecycle](doc/reference/component.md#lifecycle)
 - [Concurrency Model](doc/reference/concurrency_model.md)
+- [Dev mode](doc/reference/app.md#dev-mode)
 - [Dynamic sub components](doc/reference/component.md#dynamic-sub-components)
 - [Environment](doc/reference/environment.md)
 - [Error Handling](doc/reference/error_handling.md)

doc/miscellaneous/organizing_code.md

@@ -0,0 +1,380 @@
+# 🦉 Organising Owl code 🦉
+
+## Content
+
+- [Overview](#overview)
+- [Reactivity](#reactivity)
+- [Asynchrony](#asynchrony)
+- [Basic design: Components and Props](#basic-design-components-and-props)
+- [More advanced design: using the environment](#more-advanced-design-using-the-environment)
+- [Even more advanced: completely separating model management](#even-more-advanced-completely-separating-model-management)
+- [Special case: bypassing reactivity](#special-case-bypassing-reactivity)
+
+## Overview
+
+The topic of this document is to discuss how one could organize an Owl
+application/component/feature/codebase. Clearly, good practices for a system depends
+on its properties/semantics. For Owl 2.x applications, the most important
+ideas to have in mind are the following:
+
+- Owl provides a way to structure a component tree,
+- communication between a parent and a child component is mostly done with props
+  (and callback props to go from child to parent),
+- there is an additional communication channel: information can be propagated
+  through the environment,
+- Owl implements a reactivity system, which tracks each components and values,
+  so that it is able to only rerender the relevant subset of components,
+- asynchrony: owl components can delay renderings with `onWillStart`/`onWillUpdateProps`
+- renderings are batched (10 calls to render in the same call stack will only
+  result in 1 actual rendering)
+
+## Reactivity
+
+With the reactivity system, each value created with the `useState` or the `reactive`
+function is a proxy that allows Owl to trask which keys/values have been read
+by which components. This means that Owl can then only update the components that
+are impacted by a state change.
+
+There are two important reactivity primitives:
+
+- `useState`: a hook to turn a value into a reactive value, linked to a component
+  (so, each time the value is updated, the component is rerendered)
+- `reactive`: a function to create a "standalone" reactive value
+
+Also, it is good to know that one can call `useState` on a reactive value: this will
+create another reactive value, connected to the same source, but linked to a
+different component.
+
+Let us consider the following component tree:
+
+```mermaid
+graph TD
+    A-->B
+    A-->C
+    B-->D
+    B-->E
+    B-->F
+    C-->G
+    C-->H
+```
+
+It is important to know how renderings are applied by Owl:
+
+- if a render is initiated in A, it will render its template
+- whenever it encounter a child component (here, B and C), it will shallow
+  compare the before and after props. If they are different, Owl will render the
+  corresponding child component, otherwise it will stop.
+- each props object received by a (child) component is turned into a reactive
+  object.
+
+Each (non primitive) value given by `A` to `B` and `C` will be transformed into
+a reactive object.
+basically, owl does a useState on props at each level, and compare
+shallowly values
+
+Notes:
+
+- each component can be independently rendered (so, for example, only B and D)
+- one can create reactive objects (just like useState, but not linked to any
+  component) with the `reactive` function
+- escape hatch: one can mark an object as "raw" (non reactive) with `markRaw`, and get the target
+  object (with `toRaw`)
+- note that it is bad practice to keep reference to the raw object: doing so means
+  that some => may miss
+  rendering
+
+- note: reactivity is not free. It may be expensive for some large objects.
+
+## Asynchrony
+
+explain: onWillStart and onWillUpdateProps are async => can delay render
+
+good usecase for lazyloading libs
+
+note: most of the time, update should be done in an atomic way:
+
+```js
+// bad => will cause extra rendering, possibly corrupt
+this.state.something = someValue;
+const otherValue = await this.fetchSomething();
+this.state.otherValue = otherValue;
+```
+
+```js
+// good
+const otherValue = await this.fetchSomething();
+this.state.something = someValue;
+this.state.otherValue = otherValue;
+```
+
+```js
+// bad => will cause extra rendering, possibly corrupt
+const someValue = getsomeValue();
+const otherValue = await this.fetchSomething();
+Object.assign(this.state, {
+  someValue,
+  otherValue,
+});
+```
+
+other note: minimize async stuff
+
+```js
+setup() {
+    // strategy 1: have a loading screen
+    this.state = useState({loading: true});
+}
+```
+
+```xml
+<t t-if="state.loading">
+</t>
+...
+```
+
+// other strategy: not display a loading indicator, and update the UI atomically
+
+```js
+setup() {
+    onWillStart(() => this.loadData());
+    onWillUpdateProps(() => this.updateData());
+}
+```
+
+## Baseline design: Components and props
+
+Here is what I suppose many Owl systems will look like: `useState` in some components,
+state is propagated down the tree with props, and potentially callbacks as well
+(to communicate from child to parent)
+
+     A
+
+/ \
+ B C
+/ | \ | \
+D E F G H
+
+```js
+// in A (and maybe some/all other components)
+this.state = useState(...);
+```
+
+```xml
+<!-- in A -->
+<div>
+    <t t-if="state.message">
+        <span t-esc="state.message"/>
+    </t>
+    <B record="state.records[3]" updateValue.bind="updateValue">
+</div>
+```
+
+Notice the `.bind` suffix here, very convenient.
+
+```xml
+<!-- in B. It is now subscribed to the 'some_field' value -->
+<span t-esc="props.record.some_field"/>
+```
+
+With reactivity, any update to the state will only rerender the affected components.
+So, if some code in `A` update the value of `some_field` (without reading it!),
+only B will be rerendered.
+
+## More advanced design: passing stuff through the environment
+
+It may be tedious/impractical to pass props all along the tree when we have a deep
+component tree.
+
+One can then use the environment to communicate to children. Only issue with that
+is that stuff in the environment bypass the reactivity system: if it comes from
+a `useState` in `A` => only `A` will react to state changes, not the children that
+reads it.
+
+To solve that, one need to use `useState` on it:
+
+```js
+// in root component A
+class A extends Component {
+  setup() {
+    this.posModel = {
+      state: useState(this.state),
+      updateSomeValue: () => this.updateSomeValue(),
+      doSomethingElse: () => this.doSomethingElse(),
+    };
+    useSubEnv({ posModel: this.posModel });
+  }
+}
+
+// in some child component D
+class D extends Component {
+  setup() {
+    // this one is wrong! this.posModel.state is stated
+    this.posModel = this.env.posModel;
+
+    // this.posModel.state === the one in A, linked to the component A
+
+    // this one is ok!
+    this.posModel = useState(this.env.posModel);
+  }
+}
+```
+
+Note that we can slightly simplify the use of the model by a hook:
+
+```js
+function usePosModel() {
+  const env = useEnv();
+  const model = env.posModel;
+  return useState(model);
+}
+```
+
+This is not much simpler, but it makes sure that one does not forget to call
+`useState` on the model from the environment.
+
+## Even more advanced: completely separating model management
+
+The baseline design above it nice for many usecases, but we sometimes may want
+to separate the "model" management from the UI, especially as the model is doing
+more complicated stuff. Useful to code some complex transitions.
+
+Here is how we could use the environment and reactive object to organize code:
+
+```js
+// in my_model.js
+class MyModel {
+    ...
+    constructor() {
+        this.complicatedCacheObject = {};
+
+        markRaw(this.complicatedCacheObject);
+    }
+
+    async load(...) {
+
+    }
+
+    async update(...) {
+
+    }
+
+    async doSomeComplexThing() {
+        this.loading = true;
+
+        // coordinate here some thing
+        // fetch stuff...
+
+        this.loading = false;
+        this.data = ...;
+        ...
+    }
+    ...
+}
+
+// in root component A:
+
+class A extends Component {
+    setup() {
+        const model = useState(new MyModel(...));
+        useSubEnv({ model });
+    }
+}
+
+// in some child component:
+class D extends Component {
+    setup() {
+        // notice the useState: this is necessary to bind model changes to the
+        // component D
+        this.model = useState(this.env.model);
+    }
+}
+
+```
+
+Notes:
+
+- model could be created elsewhere, in a service, in the start code, ...
+- one may want to mark some internal stuff as 'raw'...
+- if created elsewhere, it should probably be done with `reactive`
+- the call to `reactive` could be done in the model constructor directly:
+
+  ```js
+  class MyModel {
+      constructor() {
+          ...
+          return reactive(this);
+      }
+  }
+  ```
+
+- one could slightly simplify the model use with a hook:
+
+```js
+function useModel() {
+  const env = useEnv();
+  const model = env.model;
+  return useState(model);
+}
+```
+
+This can be simply used like this in a child component:
+
+```js
+setup() {
+    this.model = useModel();
+}
+```
+
+## Special case: bypassing reactivity
+
+Sometimes, reactivity is not what we want. For example, the o_spreadsheet
+library has a huge extremely complex model, and each state transition may
+potentially update the full UI, so it needs to be fast anyway, and reactivity
+has a cost that may outweight the benefits.
+
+The strategy here is:
+
+- mark the model as raw (not necessary in all cases, but potentially important
+  if model is passed through some props)
+- find a way to be notified on update change (for example, by an event triggered
+  by an event bus)
+- perform a deep rendering: render(true), at the root component.
+
+Here is how one could organize the code then:
+
+```js
+class MainModel extends EventBus{
+    constructor() {
+        markRaw(this); // make sure this object will never be turned into a reactive
+    }
+
+    doSomething() {
+        ...
+        // need to notify the outside world that a change occurred
+        this.trigger("UPDATE");
+    }
+    ...
+}
+
+// in root component:
+class A extends Component {
+    setup() {
+        const model = new MainModel();
+
+        // notice the 'render(true)', to do a deep render
+        model.addEventListener("UPDATE", () => this.render(true));
+        useSubEnv({ model });
+    }
+}
+
+// in child components:
+class E extends Component {
+    setup() {
+        this.model = this.env.model; // not really necessary, but maybe nice
+    }
+
+    onClick() {
+        this.model.doSomething();
+    }
+}
+```

doc/reference/app.md

@@ -51,11 +51,10 @@ The `config` object is an object with some of the following keys:
 
 - **`env (object)`**: if given, this will be the shared `env` given to each component
 - **`props (object)`**: the props given to the root component
-- **`dev (boolean, default=false)`**: if `true`, the application is rendered in `dev`
+- **`dev (boolean, default=false)`**: if `true`, the application is rendered in
-  mode, which activates some additional checks (in particular, the props validation
+  [`dev` mode](#dev-mode);
-  code is only performed in dev mode)
 - **`test (boolean, default=false)`**: `test` mode is the same as `dev` mode, except
-  that Owll will not log a message to warn that Owl is in `dev` mode.
+  that Owl will not log a message to warn that Owl is in `dev` mode.
 - **`translatableAttributes (string[])`**: a list of additional attributes that should
   be translated (see [translations](translations.md))
 - **`translateFn (function)`**: a function that will be called by owl to translate
@@ -110,3 +109,11 @@ const { loadFile, mount } = owl;
   mount(Root, document.body, { env });
 })();
 ```
+
+## Dev mode
+
+Dev mode activates some additional checks and developer amenities:
+
+- [Props validation](./props.md#props-validation) is performed
+- [t-foreach](./templates.md#loops) loops check for key unicity
+- Lifecycle hooks are wrapped to report their errors in a more developer-friendly way

src/component/component_node.ts

@@ -58,13 +58,6 @@ export function useState<T extends object>(state: T): Reactive<T> | NonReactive<
     batchedRenderFunctions.set(node, render);
     // manual implementation of onWillDestroy to break cyclic dependency
     node.willDestroy.push(clearReactivesForCallback.bind(null, render));
-    if (node.app.dev) {
-      Object.defineProperty(node, "subscriptions", {
-        get() {
-          return getSubscriptions(render);
-        },
-      });
-    }
   }
   return reactive(state, render);
 }
@@ -394,4 +387,16 @@ export class ComponentNode<P extends object = any, E = any> implements VNode<Com
       }
     }
   }
+
+  // ---------------------------------------------------------------------------
+  // Some debug helpers
+  // ---------------------------------------------------------------------------
+  get name(): string {
+    return this.component.constructor.name;
+  }
+
+  get subscriptions(): ReturnType<typeof getSubscriptions> {
+    const render = batchedRenderFunctions.get(this);
+    return render ? getSubscriptions(render) : [];
+  }
 }

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();
+      if (cleanup) {
-      cleanup = effect(...dependencies) || NO_OP;
+        cleanup();
+      }
+      cleanup = effect(...dependencies);
     }
   });
 
-  onWillUnmount(() => cleanup());
+  onWillUnmount(() => cleanup && cleanup());
 }
 
 // -----------------------------------------------------------------------------

src/reactivity.ts

@@ -308,6 +308,28 @@ function makeIteratorObserver(
     }
   };
 }
+/**
+ * Creates a forEach function that will delegate to forEach on the underlying
+ * collection while observing key changes, and keys as they're iterated over,
+ * and making the passed keys/values reactive.
+ *
+ * @param target @see reactive
+ * @param callback @see reactive
+ */
+function makeForEachObserver(target: any, callback: Callback) {
+  return function forEach(forEachCb: (val: any, key: any, target: any) => void, thisArg: any) {
+    observeTargetKey(target, KEYCHANGES, callback);
+    target.forEach(function (val: any, key: any, targetObj: any) {
+      observeTargetKey(target, key, callback);
+      forEachCb.call(
+        thisArg,
+        possiblyReactive(val, callback),
+        possiblyReactive(key, callback),
+        possiblyReactive(targetObj, callback)
+      );
+    }, thisArg);
+  };
+}
 /**
  * Creates a function that will delegate to an underlying method, and check if
  * that method has modified the presence or value of a key, and notify the
@@ -370,6 +392,7 @@ const rawTypeToFuncHandlers = {
     values: makeIteratorObserver("values", target, callback),
     entries: makeIteratorObserver("entries", target, callback),
     [Symbol.iterator]: makeIteratorObserver(Symbol.iterator, target, callback),
+    forEach: makeForEachObserver(target, callback),
     clear: makeClearNotifier(target),
     get size() {
       observeTargetKey(target, KEYCHANGES, callback);
@@ -385,6 +408,7 @@ const rawTypeToFuncHandlers = {
     values: makeIteratorObserver("values", target, callback),
     entries: makeIteratorObserver("entries", target, callback),
     [Symbol.iterator]: makeIteratorObserver(Symbol.iterator, target, callback),
+    forEach: makeForEachObserver(target, callback),
     clear: makeClearNotifier(target),
     get size() {
       observeTargetKey(target, KEYCHANGES, callback);

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;
+    });
   });
 });

tests/reactivity.test.ts

@@ -1245,6 +1245,33 @@ describe("Collections", () => {
       reactiveObj.a = 1; // setting same value again shouldn't notify
       expect(observer).toHaveBeenCalledTimes(1);
     });
+
+    test("iterating with forEach returns reactives", async () => {
+      const keyObj = { a: 2 };
+      const thisArg = {};
+      const observer = jest.fn();
+      const state = reactive(new Set([keyObj]), observer);
+      let reactiveKeyObj: any, reactiveValObj: any, thisObj: any, mapObj: any;
+      state.forEach(function (this: any, val, key, map) {
+        [reactiveValObj, reactiveKeyObj, mapObj, thisObj] = [val, key, map, this];
+      }, thisArg);
+      expect(reactiveKeyObj).not.toBe(keyObj);
+      expect(reactiveValObj).not.toBe(keyObj);
+      expect(mapObj).toBe(state); // third argument should be the reactive
+      expect(thisObj).toBe(thisArg); // thisArg should not be made reactive
+      expect(toRaw(reactiveKeyObj as any)).toBe(keyObj);
+      expect(toRaw(reactiveValObj as any)).toBe(keyObj);
+      expect(reactiveKeyObj).toBe(reactiveValObj); // reactiveKeyObj and reactiveValObj should be the same object
+      reactiveKeyObj!.a = 0;
+      reactiveValObj!.a = 0;
+      expect(observer).toHaveBeenCalledTimes(0);
+      reactiveKeyObj!.a; // observe key "a" in key sub-reactive;
+      reactiveKeyObj!.a = 1;
+      expect(observer).toHaveBeenCalledTimes(1);
+      reactiveKeyObj!.a = 1; // setting same value again shouldn't notify
+      reactiveValObj!.a = 1;
+      expect(observer).toHaveBeenCalledTimes(1);
+    });
   });
 
   describe("WeakSet", () => {
@@ -1467,6 +1494,36 @@ describe("Collections", () => {
       reactiveValObj.a = 1;
       expect(observer).toHaveBeenCalledTimes(2);
     });
+
+    test("iterating with forEach returns reactives", async () => {
+      const keyObj = { a: 2 };
+      const valObj = { a: 2 };
+      const thisArg = {};
+      const observer = jest.fn();
+      const state = reactive(new Map([[keyObj, valObj]]), observer);
+      let reactiveKeyObj: any, reactiveValObj: any, thisObj: any, mapObj: any;
+      state.forEach(function (this: any, val, key, map) {
+        [reactiveValObj, reactiveKeyObj, mapObj, thisObj] = [val, key, map, this];
+      }, thisArg);
+      expect(reactiveKeyObj).not.toBe(keyObj);
+      expect(reactiveValObj).not.toBe(valObj);
+      expect(mapObj).toBe(state); // third argument should be the reactive
+      expect(thisObj).toBe(thisArg); // thisArg should not be made reactive
+      expect(toRaw(reactiveKeyObj as any)).toBe(keyObj);
+      expect(toRaw(reactiveValObj as any)).toBe(valObj);
+      reactiveKeyObj!.a = 0;
+      reactiveValObj!.a = 0;
+      expect(observer).toHaveBeenCalledTimes(0);
+      reactiveKeyObj!.a; // observe key "a" in key sub-reactive;
+      reactiveKeyObj!.a = 1;
+      expect(observer).toHaveBeenCalledTimes(1);
+      reactiveValObj!.a; // observe key "a" in val sub-reactive;
+      reactiveValObj!.a = 1;
+      expect(observer).toHaveBeenCalledTimes(2);
+      reactiveKeyObj!.a = 1; // setting same value again shouldn't notify
+      reactiveValObj!.a = 1;
+      expect(observer).toHaveBeenCalledTimes(2);
+    });
   });
 
   describe("WeakMap", () => {