[REF] rework beforeRemove code

movahhedi · Aug 28, 2021 · 21c3f61 · 21c3f61
1 parent 0fa8c41
commit 21c3f61
Show file tree

Hide file tree

Showing 8 changed files with 70 additions and 43 deletions.
diff --git a/README.md b/README.md
@@ -8,6 +8,7 @@ A very fast virtual dom library
   - [Manipulating vnodes](#manipulating-vnodes)
   - [Creating vnodes](#creating-vnodes)
   - [Configuration](#configuration)
+- [Extending `blockdom`](#extending-blockdom)
 - [Performance Notes](#performance-notes)
 - [About this project](#about-this-project)
 - [Credits](#credits)
@@ -338,6 +339,32 @@ Here is a list of every configuration options in `blockdom`:
   `blockdom` has different needs (for example, checking if a component is still
   alive).
 
+## Extending `blockdom`
+
+`blockdom` is designed to be used as a lower layer in a "real" framework. Some
+frameworks (for example, Owl, which is the one I am working on) will need a way
+to add some kind of component system. To do that, it seems like the best way is
+to add new types of vnodes.
+
+Here is the interface of a blockdom vnode:
+
+```js
+export interface VNode<T = any> {
+  mount(parent: HTMLElement, afterNode: Node | null): void;
+  moveBefore(other: T | null, afterNode: Node | null): void;
+  patch(other: T, withBeforeRemove: boolean): void;
+  beforeRemove(): void;
+  remove(): void;
+  firstNode(): Node | undefined;
+}
+```
+
+So, to add a new vnode type, we simply need to define an object or a class with
+these methods, and it will work with the `mount/patch/remove` methods.
+
+Notice the `beforeRemove` method: it is a method used to let the framework know
+that a vnode will be removed. It is called before the node is removed.
+
 ## Performance Notes
 
 `blockdom` is very fast, I believe. If you read this section, you may be interested

diff --git a/src/block.ts b/src/block.ts
@@ -555,6 +555,7 @@ function createBlockClass(
     }
 
     beforeRemove() {}
+
     remove() {
       elementRemove.call(this.el);
     }
@@ -611,7 +612,7 @@ function createBlockClass(
       this.el = el as HTMLElement;
       this.parentEl = parent;
     }
-    patch(other: Block) {
+    patch(other: Block, withBeforeRemove: boolean) {
       // note: we don't check for equality here. It should be done by the caller
       if (this === other) {
         return;
@@ -646,9 +647,11 @@ function createBlockClass(
           const child2 = children2![i];
           if (child1) {
             if (child2) {
-              child1.patch(child2);
+              child1.patch(child2, withBeforeRemove);
             } else {
-              child1.beforeRemove();
+              if (withBeforeRemove) {
+                child1.beforeRemove();
+              }
               child1.remove();
               children1[i] = undefined;
             }

diff --git a/src/index.ts b/src/index.ts
@@ -10,7 +10,7 @@ export { html } from "./html";
 export interface VNode<T = any> {
   mount(parent: HTMLElement, afterNode: Node | null): void;
   moveBefore(other: T | null, afterNode: Node | null): void;
-  patch(other: T): void;
+  patch(other: T, withBeforeRemove: boolean): void;
   beforeRemove(): void;
   remove(): void;
   firstNode(): Node | undefined;
@@ -27,12 +27,14 @@ export function mount(vnode: VNode, fixture: HTMLElement) {
   vnode.mount(fixture, null);
 }
 
-export function patch(vnode1: VNode, vnode2: VNode) {
-  vnode1.patch(vnode2);
+export function patch(vnode1: VNode, vnode2: VNode, withBeforeRemove: boolean = false) {
+  vnode1.patch(vnode2, withBeforeRemove);
 }
 
-export function remove(vnode: VNode) {
-  vnode.beforeRemove();
+export function remove(vnode: VNode, withBeforeRemove: boolean = false) {
+  if (withBeforeRemove) {
+    vnode.beforeRemove();
+  }
   vnode.remove();
 }
 

diff --git a/src/list.ts b/src/list.ts
@@ -17,11 +17,9 @@ class VList {
   anchor: Node | undefined;
   parentEl?: HTMLElement | undefined;
   singleNode?: boolean | undefined;
-  withBeforeRemove: boolean;
 
-  constructor(children: VNode[], withBeforeRemove: boolean) {
+  constructor(children: VNode[]) {
     this.children = children;
-    this.withBeforeRemove = withBeforeRemove;
   }
 
   mount(parent: HTMLElement, afterNode: Node | null) {
@@ -44,7 +42,7 @@ class VList {
     // todo
   }
 
-  patch(other: VList) {
+  patch(other: VList, withBeforeRemove: boolean) {
     if (this === other) {
       return;
     }
@@ -66,7 +64,6 @@ class VList {
 
     const _anchor = this.anchor!;
     const isOnlyChild = this.singleNode;
-    const withBeforeRemove = this.withBeforeRemove;
     const parent = this.parentEl!;
 
     // fast path: no new child => only remove
@@ -110,7 +107,7 @@ class VList {
       let startKey1 = startVn1.key;
       let startKey2 = startVn2.key;
       if (startKey1 === startKey2) {
-        cPatch.call(startVn1, startVn2);
+        cPatch.call(startVn1, startVn2, withBeforeRemove);
         ch2[startIdx2] = startVn1;
         startVn1 = ch1[++startIdx1];
         startVn2 = ch2[++startIdx2];
@@ -120,7 +117,7 @@ class VList {
       let endKey1 = endVn1.key;
       let endKey2 = endVn2.key;
       if (endKey1 === endKey2) {
-        cPatch.call(endVn1, endVn2);
+        cPatch.call(endVn1, endVn2, withBeforeRemove);
         ch2[endIdx2] = endVn1;
         endVn1 = ch1[--endIdx1];
         endVn2 = ch2[--endIdx2];
@@ -129,7 +126,7 @@ class VList {
       // -------------------------------------------------------------------
       if (startKey1 === endKey2) {
         // bnode moved right
-        cPatch.call(startVn1, endVn2);
+        cPatch.call(startVn1, endVn2, withBeforeRemove);
         ch2[endIdx2] = startVn1;
         const nextChild = ch2[endIdx2 + 1];
         cMoveBefore.call(startVn1, nextChild, _anchor);
@@ -140,7 +137,7 @@ class VList {
       // -------------------------------------------------------------------
       if (endKey1 === startKey2) {
         // bnode moved left
-        cPatch.call(endVn1, startVn2);
+        cPatch.call(endVn1, startVn2, withBeforeRemove);
         ch2[startIdx2] = endVn1;
         const nextChild = ch1[startIdx1];
         cMoveBefore.call(endVn1, nextChild, _anchor);
@@ -156,7 +153,7 @@ class VList {
       } else {
         const elmToMove = ch1[idxInOld];
         cMoveBefore.call(elmToMove, startVn1, null);
-        cPatch.call(elmToMove, startVn2);
+        cPatch.call(elmToMove, startVn2, withBeforeRemove);
         ch2[startIdx2] = elmToMove;
         ch1[idxInOld] = null as any;
       }
@@ -185,14 +182,12 @@ class VList {
   }
 
   beforeRemove() {
-    if (this.withBeforeRemove) {
-      const children = this.children;
-      const l = children.length;
-      if (l) {
-        const beforeRemove = children[0].beforeRemove;
-        for (let i = 0; i < l; i++) {
-          beforeRemove.call(children[i]);
-        }
+    const children = this.children;
+    const l = children.length;
+    if (l) {
+      const beforeRemove = children[0].beforeRemove;
+      for (let i = 0; i < l; i++) {
+        beforeRemove.call(children[i]);
       }
     }
   }
@@ -223,8 +218,8 @@ class VList {
   }
 }
 
-export function list(children: VNode[], withBeforeRemove: boolean = false): VNode<VList> {
-  return new VList(children, withBeforeRemove);
+export function list(children: VNode[]): VNode<VList> {
+  return new VList(children);
 }
 
 function createMapping(ch1: any[], startIdx1: number, endIdx2: number): { [key: string]: any } {

diff --git a/src/multi.ts b/src/multi.ts
@@ -10,10 +10,6 @@ const nodeRemoveChild = nodeProto.removeChild;
 // Multi NODE
 // -----------------------------------------------------------------------------
 
-// TODO!!!!!
-// todo:  either keep a child or a anchor, but not both
-// and use same array!!!, and replacechild
-
 class VMulti {
   children: (VNode | undefined)[];
   anchors?: Node[] | undefined;
@@ -61,7 +57,7 @@ class VMulti {
     }
   }
 
-  patch(other: VMulti) {
+  patch(other: VMulti, withBeforeRemove: boolean) {
     if (this === other) {
       return;
     }
@@ -74,13 +70,15 @@ class VMulti {
       const vn2 = children2[i];
       if (vn1) {
         if (vn2) {
-          vn1.patch(vn2);
+          vn1.patch(vn2, withBeforeRemove);
         } else {
           const afterNode = vn1.firstNode()!;
           const anchor = document.createTextNode("");
           anchors[i] = anchor;
           nodeInsertBefore.call(parentEl, anchor, afterNode);
-          vn1.beforeRemove();
+          if (withBeforeRemove) {
+            vn1.beforeRemove();
+          }
           vn1.remove();
           children1[i] = undefined;
         }

diff --git a/src/toggler.ts b/src/toggler.ts
@@ -25,17 +25,19 @@ class VToggler {
     this.child.moveBefore(other ? other.child : null, afterNode);
   }
 
-  patch(other: VToggler) {
+  patch(other: VToggler, withBeforeRemove: boolean) {
     if (this === other) {
       return;
     }
     let child1 = this.child;
     let child2 = other.child;
     if (this.key === other.key) {
-      child1.patch(child2);
+      child1.patch(child2, withBeforeRemove);
     } else {
       child2.mount(this.parentEl!, child1.firstNode()!);
-      child1.beforeRemove();
+      if (withBeforeRemove) {
+        child1.beforeRemove();
+      }
       child1.remove();
       this.child = child2;
       this.key = other.key;

diff --git a/tests/block_attributes.test.ts b/tests/block_attributes.test.ts
@@ -95,7 +95,7 @@ describe("properties", () => {
 
     // rerender with a different value, and patch actual dom, to check that
     // input value was properly reset by owl
-    tree.patch(block(["potato"]));
+    patch(tree, block(["potato"]));
     expect(input.value).toBe("potato");
   });
 

diff --git a/tests/remove_hook.test.ts b/tests/remove_hook.test.ts
@@ -27,7 +27,7 @@ describe("before remove is called", () => {
 
     let n = 1;
     child.beforeRemove = () => n++;
-    patch(tree, block1([], []));
+    patch(tree, block1([], []), true);
     expect(fixture.innerHTML).toBe("<div></div>");
     expect(n).toBe(2);
   });
@@ -43,7 +43,7 @@ describe("before remove is called", () => {
 
     let n = 1;
     child.beforeRemove = () => n++;
-    patch(tree, block1([], []));
+    patch(tree, block1([], []), true);
     expect(fixture.innerHTML).toBe("<p></p>");
     expect(n).toBe(2);
   });
@@ -60,7 +60,7 @@ describe("before remove is called", () => {
 
     let n = 1;
     child1.beforeRemove = () => n++;
-    patch(tree, block1([], [multi([])]));
+    patch(tree, block1([], [multi([])]), true);
     expect(fixture.innerHTML).toBe("<div></div>");
     expect(n).toBe(2);
   });
@@ -77,7 +77,7 @@ describe("before remove is called", () => {
 
     let n = 1;
     child1.beforeRemove = () => n++;
-    patch(tree, block1([], []));
+    patch(tree, block1([], []), true);
     expect(fixture.innerHTML).toBe("<div></div>");
     expect(n).toBe(2);
   });