From 5d9cff0331e3340b37a15b429b74eda4e7750e65 Mon Sep 17 00:00:00 2001
From: Samuel Degueldre <sad@odoo.com>
Date: Wed, 27 Apr 2022 10:27:57 +0200
Subject: [PATCH] [IMP] app: allow duplicate templates iff they are the same

Previously, we used an option to allow duplicate templates, if that
option was passed, we would always replace the existing template with
the new template, and if it wasn't, we would always throw an error even
if the template's content was the same.

Allowing to replace a template with a different one is problematic, as
it may or may not have already been compiled, which may cause various
problems. On the other hand, if the template is the same, there is no
point in throwing an error, as we can just silently ignore the second
addition.
---
 src/app/template_set.ts               | 20 +++++++++++---------
 tests/compiler/error_handling.test.ts |  9 ---------
 tests/compiler/validation.test.ts     |  9 +++++----
 3 files changed, 16 insertions(+), 22 deletions(-)

diff --git a/src/app/template_set.ts b/src/app/template_set.ts
index c043f19a..a7e5e394 100644
--- a/src/app/template_set.ts
+++ b/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);
     }
   }
 
diff --git a/tests/compiler/error_handling.test.ts b/tests/compiler/error_handling.test.ts
index 4371b08c..f9cebdfd 100644
--- a/tests/compiler/error_handling.test.ts
+++ b/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(() => {
diff --git a/tests/compiler/validation.test.ts b/tests/compiler/validation.test.ts
index 9c3499a6..98531121 100644
--- a/tests/compiler/validation.test.ts
+++ b/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");
   });