From b4f84513a637f1ce10d47ecceb32537e07e2ce36 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?G=C3=A9ry=20Debongnie?= <ged@odoo.com>
Date: Tue, 29 Oct 2024 13:47:52 +0100
Subject: [PATCH] [REF] app: reorganize startup code

The goal of this refactoring is to delay the instantiation of the
component node until we are ready to mount the component. This is useful
so we know if a component will be attached or mounted (which means that
it needs to have a template, or not)
---
 src/runtime/app.ts                            | 72 ++++++++++---------
 tests/app/__snapshots__/sub_root.test.ts.snap | 11 ---
 tests/components/props_validation.test.ts     |  2 +-
 3 files changed, 40 insertions(+), 45 deletions(-)

diff --git a/src/runtime/app.ts b/src/runtime/app.ts
index 9431e6e7..ea6e9fd4 100644
--- a/src/runtime/app.ts
+++ b/src/runtime/app.ts
@@ -53,7 +53,7 @@ declare global {
 }
 
 interface Root<P extends Props, E> {
-  node: ComponentNode<P, E>;
+  node: ComponentNode<P, E> | null;
   mount(target: HTMLElement | ShadowRoot, options?: MountOptions): Promise<Component<P, E>>;
   destroy(): void;
 }
@@ -74,8 +74,8 @@ export class App<
   props: P;
   env: E;
   scheduler = new Scheduler();
-  subRoots: Set<ComponentNode> = new Set();
-  root: ComponentNode<P, E> | null = null;
+  subRoots: Set<Root<any, any>> = new Set();
+  root: Root<P, E> | null = null;
   warnIfNoStaticProps: boolean;
 
   constructor(Root: ComponentConstructor<P, E>, config: AppConfig<P, E> = {}) {
@@ -101,10 +101,8 @@ export class App<
     target: HTMLElement | ShadowRoot,
     options?: MountOptions
   ): Promise<Component<P, E> & InstanceType<T>> {
-    const root = this.createRoot(this.Root, { props: this.props });
-    this.root = root.node;
-    this.subRoots.delete(root.node);
-    return root.mount(target, options) as any;
+    this.root = this.createRoot(this.Root, { props: this.props });
+    return this.root.mount(target, options) as any;
   }
 
   createRoot<Props extends object, SubEnv = any>(
@@ -112,25 +110,24 @@ export class App<
     config: RootConfig<Props, SubEnv> = {}
   ): Root<Props, SubEnv> {
     const props = config.props || ({} as Props);
-    // hack to make sure the sub root get the sub env if necessary. for owl 3,
-    // would be nice to rethink the initialization process to make sure that
-    // we can create a ComponentNode and give it explicitely the env, instead
-    // of looking it up in the app
     const env = this.env;
-    if (config.env) {
-      this.env = config.env as any;
-    }
-
-    const restore = saveCurrent();
-    const node = this.makeNode(Root, props);
-    restore();
-    if (config.env) {
-      this.env = env;
-    }
-    this.subRoots.add(node);
-    return {
-      node,
+    const root: Root<Props, SubEnv> = {
+      node: null,
       mount: (target: HTMLElement | ShadowRoot, options?: MountOptions) => {
+        // hack to make sure the sub root get the sub env if necessary. for owl 3,
+        // would be nice to rethink the initialization process to make sure that
+        // we can create a ComponentNode and give it explicitely the env, instead
+        // of looking it up in the app
+        if (config.env) {
+          this.env = config.env as any;
+        }
+        const restore = saveCurrent();
+        const node = this.makeNode(Root, props);
+        root.node = node;
+        restore();
+        if (config.env) {
+          this.env = env;
+        }
         App.validateTarget(target);
         if (this.dev) {
           validateProps(Root, props, { __owl__: { app: this } });
@@ -139,11 +136,16 @@ export class App<
         return prom;
       },
       destroy: () => {
-        this.subRoots.delete(node);
-        node.destroy();
-        this.scheduler.processTasks();
+        this.subRoots.delete(root);
+        if (root.node) {
+          root.node?.destroy();
+          this.scheduler.processTasks();
+        }
       },
     };
+
+    this.subRoots.add(root);
+    return root;
   }
 
   makeNode(Component: ComponentConstructor, props: any): ComponentNode {
@@ -178,13 +180,17 @@ export class App<
   }
 
   destroy() {
-    if (this.root) {
-      for (let subroot of this.subRoots) {
-        subroot.destroy();
-      }
-      this.root.destroy();
-      this.scheduler.processTasks();
+    const roots = [...this.subRoots].reverse();
+    for (let root of roots) {
+      root.destroy();
     }
+    // if (this.root) {
+    //   for (let subroot of this.subRoots) {
+    //     subroot.destroy();
+    //   }
+    //   this.root.destroy();
+    this.scheduler.processTasks();
+    // }
     apps.delete(this);
   }
 
diff --git a/tests/app/__snapshots__/sub_root.test.ts.snap b/tests/app/__snapshots__/sub_root.test.ts.snap
index efc4d563..aa5ece6c 100644
--- a/tests/app/__snapshots__/sub_root.test.ts.snap
+++ b/tests/app/__snapshots__/sub_root.test.ts.snap
@@ -94,17 +94,6 @@ exports[`subroot can create a root in a setup function, then use a hook 1`] = `
 }"
 `;
 
-exports[`subroot can create a root in a setup function, then use a hook 2`] = `
-"function anonymous(app, bdom, helpers
-) {
-  let { text, createBlock, list, multi, html, toggler, comment } = bdom;
-  
-  return function template(ctx, node, key = \\"\\") {
-    return text(\`c\`);
-  }
-}"
-`;
-
 exports[`subroot can mount subroot 1`] = `
 "function anonymous(app, bdom, helpers
 ) {
diff --git a/tests/components/props_validation.test.ts b/tests/components/props_validation.test.ts
index b1b8e81b..727535f1 100644
--- a/tests/components/props_validation.test.ts
+++ b/tests/components/props_validation.test.ts
@@ -702,7 +702,7 @@ describe("props validation", () => {
     const app = new App(Parent, { test: true });
     await app.mount(fixture);
     expect(fixture.innerHTML).toBe("12");
-    expect(app.root!.subscriptions).toEqual([{ keys: ["otherValue"], target: obj }]);
+    expect(app.root!.node!.subscriptions).toEqual([{ keys: ["otherValue"], target: obj }]);
   });
 
   test("props are validated whenever component is updated", async () => {