[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

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@odoo/owl",
-  "version": "2.0.0-beta-6",
+  "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/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/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/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");
 
-  onWillStart(() => {
-    expect(name + ": " + status(component)).toBe(name + ": " + "new");
-    logStep(`${name}:willStart`);
-  });
+  if (!skipAsyncHooks) {
+    onWillStart(() => {
+      expect(name + ": " + status(component)).toBe(name + ": " + "new");
+      logStep(`${name}:willStart`);
+    });
+  }
 
   onMounted(() => {
     expect(name + ": " + status(component)).toBe(name + ": " + "mounted");
     logStep(`${name}:mounted`);
   });
 
-  onWillUpdateProps(() => {
-    expect(name + ": " + status(component)).toBe(name + ": " + "mounted");
-    logStep(`${name}:willUpdateProps`);
-  });
+  if (!skipAsyncHooks) {
+    onWillUpdateProps(() => {
+      expect(name + ": " + status(component)).toBe(name + ": " + "mounted");
+      logStep(`${name}:willUpdateProps`);
+    });
+  }
 
   onWillRender(() => {
     logStep(`${name}:willRender`);