[REL] v2.0.0-beta-7

# 2.0.0-beta-7

- fix: concurrency: do not render delayed fibers when cancelled
- imp: allow duplicate templates if and only if they are the same

doc/reference/component.md

@@ -76,7 +76,8 @@ The `Component` class has a very small API.
 
   By default, the render initiated by this method will stop at each child
   component if their props are (shallow) equal. To force a render to update
-  all child components, one can use the optional `deep` argument.
+  all child components, one can use the optional `deep` argument. Note that the
+  value of the `deep` argument needs to be a boolean, not a truthy value.
 
 ## Static Properties
 

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@odoo/owl",
-  "version": "2.0.0-beta-5",
+  "version": "2.0.0-beta-7",
   "description": "Odoo Web Library (OWL)",
   "main": "dist/owl.cjs.js",
   "browser": "dist/owl.iife.js",

src/app/template_set.ts

@@ -79,18 +79,20 @@ export class TemplateSet {
     this.helpers = makeHelpers(this.getTemplate.bind(this));
   }
 
-  addTemplate(
-    name: string,
-    template: string | Element,
-    options: { allowDuplicate?: boolean } = {}
-  ) {
-    if (name in this.rawTemplates && !options.allowDuplicate) {
-      throw new Error(`Template ${name} already defined`);
+  addTemplate(name: string, template: string | Element) {
+    if (name in this.rawTemplates) {
+      const rawTemplate = this.rawTemplates[name];
+      const currentAsString = typeof rawTemplate === "string" ? rawTemplate : rawTemplate.outerHTML;
+      const newAsString = typeof template === "string" ? template : template.outerHTML;
+      if (currentAsString === newAsString) {
+        return;
+      }
+      throw new Error(`Template ${name} already defined with different content`);
     }
     this.rawTemplates[name] = template;
   }
 
-  addTemplates(xml: string | Document, options: { allowDuplicate?: boolean } = {}) {
+  addTemplates(xml: string | Document) {
     if (!xml) {
       // empty string
       return;
@@ -98,7 +100,7 @@ export class TemplateSet {
     xml = xml instanceof Document ? xml : parseXML(xml);
     for (const template of xml.querySelectorAll("[t-name]")) {
       const name = template.getAttribute("t-name")!;
-      this.addTemplate(name, template, options);
+      this.addTemplate(name, template);
     }
   }
 

src/component/component.ts

@@ -37,6 +37,6 @@ export class Component<Props = any, Env = any> {
   setup() {}
 
   render(deep: boolean = false) {
-    this.__owl__.render(deep);
+    this.__owl__.render(deep === true);
   }
 }

src/component/component_node.ts

@@ -47,7 +47,7 @@ export function useState<T extends object>(state: T): Reactive<T> | NonReactive<
   const node = getCurrent();
   let render = batchedRenderFunctions.get(node)!;
   if (!render) {
-    render = batched(node.render.bind(node));
+    render = batched(node.render.bind(node, false));
     batchedRenderFunctions.set(node, render);
     // manual implementation of onWillDestroy to break cyclic dependency
     node.willDestroy.push(clearReactivesForCallback.bind(null, render));
@@ -193,7 +193,7 @@ export class ComponentNode<P extends object = any, E = any> implements VNode<Com
     }
   }
 
-  async render(deep: boolean = false) {
+  async render(deep: boolean) {
     let current = this.fiber;
     if (current && (current.root!.locked || (current as any).bdom === true)) {
       await Promise.resolve();

src/component/fibers.ts

@@ -42,6 +42,10 @@ export function makeRootFiber(node: ComponentNode): Fiber {
   return fiber;
 }
 
+function throwOnRender() {
+  throw new Error("Attempted to render cancelled fiber");
+}
+
 /**
  * @returns number of not-yet rendered fibers cancelled
  */
@@ -49,6 +53,7 @@ function cancelFibers(fibers: Fiber[]): number {
   let result = 0;
   for (let fiber of fibers) {
     let node = fiber.node;
+    fiber.render = throwOnRender;
     if (node.status === STATUS.NEW) {
       node.destroy();
     }

src/component/scheduler.ts

@@ -32,7 +32,7 @@ export class Scheduler {
       let renders = this.delayedRenders;
       this.delayedRenders = [];
       for (let f of renders) {
-        if (f.root && f.node.status !== STATUS.DESTROYED) {
+        if (f.root && f.node.status !== STATUS.DESTROYED && f.node.fiber === f) {
           f.render();
         }
       }

tests/compiler/error_handling.test.ts

@@ -12,15 +12,6 @@ describe("error handling", () => {
     expect(() => context.renderToString("invalidname")).toThrow("Missing template");
   });
 
-  test("cannot add twice the same template", () => {
-    const context = new TestContext();
-    context.addTemplate("test", `<t></t>`);
-    expect(() => context.addTemplate("test", "<div/>", { allowDuplicate: true })).not.toThrow(
-      "already defined"
-    );
-    expect(() => context.addTemplate("test", "<div/>")).toThrow("already defined");
-  });
-
   test("addTemplates throw if parser error", () => {
     const context = new TestContext();
     expect(() => {

tests/compiler/validation.test.ts

@@ -11,11 +11,12 @@ describe("basic validation", () => {
     expect(() => context.getTemplate("invalidname")).toThrow("Missing template");
   });
 
-  test("cannot add twice the same template", () => {
+  test("cannot add a different template with the same name", () => {
     const context = new TemplateSet();
-    expect(() => context.addTemplate("test", "<div/>", { allowDuplicate: true })).not.toThrow(
-      "already defined"
-    );
+    context.addTemplate("test", `<t/>`);
+    // Same template with the same name is fine
+    expect(() => context.addTemplate("test", "<t/>")).not.toThrow();
+    // Different template with the same name crashes
     expect(() => context.addTemplate("test", "<div/>")).toThrow("already defined");
   });
 

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

@@ -1111,6 +1111,56 @@ exports[`delay willUpdateProps with rendering grandchild 4`] = `
 }"
 `;
 
+exports[`delayed fiber does not get rendered if it was cancelled 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    const b2 = text(\`A\`);
+    const b3 = component(\`B\`, {}, key + \`__1\`, node, ctx);
+    return multi([b2, b3]);
+  }
+}"
+`;
+
+exports[`delayed fiber does not get rendered if it was cancelled 2`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    const b2 = text(\`B\`);
+    const b3 = component(\`C\`, {}, key + \`__1\`, node, ctx);
+    return multi([b2, b3]);
+  }
+}"
+`;
+
+exports[`delayed fiber does not get rendered if it was cancelled 3`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    const b2 = text(\`C\`);
+    const b3 = component(\`D\`, {}, key + \`__1\`, node, ctx);
+    return multi([b2, b3]);
+  }
+}"
+`;
+
+exports[`delayed fiber does not get rendered if it was cancelled 4`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    return text(\`D\`);
+  }
+}"
+`;
+
 exports[`delayed rendering, but then initial rendering is cancelled by yet another render 1`] = `
 "function anonymous(bdom, helpers
 ) {

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

@@ -127,6 +127,30 @@ exports[`rendering semantics props are reactive 2`] = `
 }"
 `;
 
+exports[`rendering semantics render need a boolean = true to be 'deep' 1`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    const b2 = text(ctx['state'].value);
+    const b3 = component(\`Child\`, {}, key + \`__1\`, node, ctx);
+    return multi([b2, b3]);
+  }
+}"
+`;
+
+exports[`rendering semantics render need a boolean = true to be 'deep' 2`] = `
+"function anonymous(bdom, helpers
+) {
+  let { text, createBlock, list, multi, html, toggler, component, comment } = bdom;
+  
+  return function template(ctx, node, key = \\"\\") {
+    return text(\`child\`);
+  }
+}"
+`;
+
 exports[`rendering semantics render with deep=true followed by render with deep=false work as expected 1`] = `
 "function anonymous(bdom, helpers
 ) {

tests/components/concurrency.test.ts

@@ -3696,6 +3696,89 @@ test("another scenario with delayed rendering", async () => {
   ]).toBeLogged();
 });
 
+test("delayed fiber does not get rendered if it was cancelled", async () => {
+  class D extends Component {
+    static template = xml`D`;
+    setup() {
+      useLogLifecycle("", true);
+    }
+  }
+
+  class C extends Component {
+    static template = xml`C<D/>`;
+    static components = { D };
+    setup() {
+      useLogLifecycle("", true);
+      c = this;
+    }
+  }
+  let c: C;
+
+  class B extends Component {
+    static template = xml`B<C/>`;
+    static components = { C };
+    setup() {
+      useLogLifecycle("", true);
+    }
+  }
+
+  class A extends Component {
+    static template = xml`A<B/>`;
+    static components = { B };
+    setup() {
+      useLogLifecycle("", true);
+    }
+  }
+
+  const a = await mount(A, fixture);
+  expect(fixture.innerHTML).toBe("ABCD");
+  expect([
+    "A:setup",
+    "A:willRender",
+    "B:setup",
+    "A:rendered",
+    "B:willRender",
+    "C:setup",
+    "B:rendered",
+    "C:willRender",
+    "D:setup",
+    "C:rendered",
+    "D:willRender",
+    "D:rendered",
+    "D:mounted",
+    "C:mounted",
+    "B:mounted",
+    "A:mounted",
+  ]).toBeLogged();
+  // Start a render in C
+  c!.render(true);
+  await nextMicroTick();
+  expect(["C:willRender", "C:rendered"]).toBeLogged();
+  // Start a render in A such that C is already rendered, but D will be delayed
+  // (because A is rendering) then cancelled (when the render from A reaches C)
+  a.render(true);
+  // Make sure the render can go to completion (Cancelled fibers will throw when rendered)
+  await nextTick();
+  expect([
+    "A:willRender",
+    "A:rendered",
+    "B:willRender",
+    "B:rendered",
+    "C:willRender",
+    "C:rendered",
+    "D:willRender",
+    "D:rendered",
+    "A:willPatch",
+    "B:willPatch",
+    "C:willPatch",
+    "D:willPatch",
+    "D:patched",
+    "C:patched",
+    "B:patched",
+    "A:patched",
+  ]).toBeLogged();
+});
+
 test("destroyed component causes other soon to be destroyed component to rerender, weird stuff happens", async () => {
   let def = makeDeferred();
   let c: any = null;

tests/components/rendering.test.ts

@@ -102,6 +102,43 @@ describe("rendering semantics", () => {
     expect(childN).toBe(2);
   });
 
+  test("render need a boolean = true to be 'deep'", async () => {
+    let childN = 0;
+    let parentN = 0;
+    class Child extends Component {
+      static template = xml`child`;
+      setup() {
+        onRendered(() => childN++);
+      }
+    }
+
+    class Parent extends Component {
+      static template = xml`
+        <t t-esc="state.value"/>
+        <Child/>
+      `;
+      static components = { Child };
+
+      state = { value: "A" };
+      setup() {
+        onRendered(() => parentN++);
+      }
+    }
+
+    const parent = await mount(Parent, fixture);
+
+    expect(fixture.innerHTML).toBe("Achild");
+    expect(parentN).toBe(1);
+    expect(childN).toBe(1);
+
+    parent.state.value = "B";
+    parent.render("true" as any as boolean);
+    await nextTick();
+    expect(fixture.innerHTML).toBe("Bchild");
+    expect(parentN).toBe(2);
+    expect(childN).toBe(1);
+  });
+
   test("render with deep=true followed by render with deep=false work as expected", async () => {
     class Child extends Component {
       static template = xml`child<t t-esc="env.getValue()"/>`;

tests/helpers.ts

@@ -138,7 +138,7 @@ const steps: string[] = [];
 export function logStep(step: string) {
   steps.push(step);
 }
-export function useLogLifecycle(key?: string) {
+export function useLogLifecycle(key?: string, skipAsyncHooks: boolean = false) {
   const component = useComponent();
   let name = component.constructor.name;
   if (key) {
@@ -147,20 +147,24 @@ export function useLogLifecycle(key?: string) {
   logStep(`${name}:setup`);
   expect(name + ": " + status(component)).toBe(name + ": " + "new");
 
+  if (!skipAsyncHooks) {
     onWillStart(() => {
       expect(name + ": " + status(component)).toBe(name + ": " + "new");
       logStep(`${name}:willStart`);
     });
+  }
 
   onMounted(() => {
     expect(name + ": " + status(component)).toBe(name + ": " + "mounted");
     logStep(`${name}:mounted`);
   });
 
+  if (!skipAsyncHooks) {
     onWillUpdateProps(() => {
       expect(name + ": " + status(component)).toBe(name + ": " + "mounted");
       logStep(`${name}:willUpdateProps`);
     });
+  }
 
   onWillRender(() => {
     logStep(`${name}:willRender`);