Merge pull request #202 from airbnb/refactor-stopPropagation

Refactor `stopPropagation` to fix #198

Author: malash

packages/core/src/events.ts

@@ -13,44 +13,30 @@ const processingEventBeforeDispatch = (e: any) => {
 export class GojiEvent {
   private instanceMap = new Map<number, ElementInstance>();
 
-  private stoppedPropagation = new Map<number, Map<string, number>>();
-
   public registerEventHandler(id: number, instance: ElementInstance) {
     this.instanceMap.set(id, instance);
   }
 
   public unregisterEventHandler(id: number) {
     this.instanceMap.delete(id);
-    this.stoppedPropagation.delete(id);
   }
 
   public triggerEvent(e: any) {
     processingEventBeforeDispatch(e);
-    const { target, currentTarget, timeStamp } = e;
-
+    const { currentTarget, timeStamp } = e;
     const id = currentTarget.dataset.gojiId;
-    const sourceId = target.dataset.gojiId;
-
     const type = camelCase(`on-${(e.type || '').toLowerCase()}`);
+
     const instance = this.instanceMap.get(id);
     if (!instance) {
       return;
     }
 
-    let stoppedPropagation = this.stoppedPropagation.get(sourceId);
-    if (stoppedPropagation && stoppedPropagation.get(type) === timeStamp) {
-      return;
-    }
-
     e.stopPropagation = () => {
-      if (!stoppedPropagation) {
-        stoppedPropagation = new Map<string, number>();
-        this.stoppedPropagation.set(sourceId, stoppedPropagation);
-      }
-      stoppedPropagation.set(type, timeStamp);
+      instance.stopPropagation(type, timeStamp ?? undefined);
     };
 
-    instance.triggerEvent(type, e);
+    instance.triggerEvent(type, timeStamp ?? undefined, e);
   }
 }
 

packages/core/src/reconciler/__tests__/instance.test.tsx

@@ -166,4 +166,82 @@ describe('ElementInstance', () => {
     setItemsCallback([2, 1, 3]);
     expect(getTextList()).toEqual(['2', '1', '3']);
   });
+
+  test.each([[true], [false]])('stopPropagation = %s', shouldStopPropagation => {
+    const onRootTap = jest.fn();
+    const onLeafTap = jest.fn();
+    const rootRef = createRef<PublicInstance>();
+    const leafRef = createRef<PublicInstance>();
+    const App = () => (
+      <View onTap={onRootTap} ref={rootRef}>
+        <View>
+          <View>
+            <View>
+              <View>
+                <View
+                  onTap={e => {
+                    if (shouldStopPropagation) {
+                      e.stopPropagation();
+                    }
+                    onLeafTap();
+                  }}
+                  ref={leafRef}
+                >
+                  Click
+                </View>
+              </View>
+            </View>
+          </View>
+        </View>
+      </View>
+    );
+    const { getContainer } = render(<App />);
+    const rootNode = (getContainer() as { meta: ElementNodeDevelopment }).meta;
+    let leafNode: ElementNodeDevelopment;
+    for (
+      leafNode = rootNode;
+      leafNode.children?.length;
+      leafNode = leafNode.children[0] as ElementNodeDevelopment
+    ) {
+      // do nothing
+    }
+
+    const timeStamp = Date.now();
+    // trigger event on leaf
+    gojiEvents.triggerEvent({
+      type: 'tap',
+      timeStamp,
+      currentTarget: {
+        dataset: {
+          gojiId: leafRef.current!.unsafe_gojiId,
+        },
+      },
+      target: {
+        dataset: {
+          gojiId: leafRef.current!.unsafe_gojiId,
+        },
+      },
+    });
+    act(() => {});
+    expect(onLeafTap).toBeCalledTimes(1);
+    expect(onRootTap).toBeCalledTimes(0);
+    // trigger event on root
+    gojiEvents.triggerEvent({
+      type: 'tap',
+      timeStamp,
+      currentTarget: {
+        dataset: {
+          gojiId: rootRef.current!.unsafe_gojiId,
+        },
+      },
+      target: {
+        dataset: {
+          gojiId: leafRef.current!.unsafe_gojiId,
+        },
+      },
+    });
+    act(() => {});
+    expect(onLeafTap).toBeCalledTimes(1);
+    expect(onRootTap).toBeCalledTimes(shouldStopPropagation ? 0 : 1);
+  });
 });

packages/core/src/reconciler/instance.ts

@@ -102,6 +102,8 @@ export class ElementInstance extends BaseInstance {
 
   public subtreeDepth?: number;
 
+  private stoppedPropagation: Map<string, number> = new Map();
+
   public getSubtreeId(): number | undefined {
     const subtreeMaxDepth = useSubtree ? subtreeMaxDepthFromConfig : Infinity;
     // wrapped component should return its wrapper as subtree id
@@ -224,13 +226,25 @@ export class ElementInstance extends BaseInstance {
     }
   }
 
-  public triggerEvent(propKey: string, data: any) {
-    const listener = this.props[propKey];
+  public triggerEvent(type: string, timeStamp: number | undefined, data: any) {
+    if (timeStamp === undefined) {
+      if (process.env.NODE_ENV !== 'production') {
+        console.warn(
+          '`triggerEvent` should be called with a `timeStamp`, otherwise it will trigger all events. This might be an internal error in GojiJS',
+        );
+      }
+      return;
+    }
+    // prevent triggering if the event has been stopped by `stopPropagation`
+    if (this.stoppedPropagation.get(type) === timeStamp) {
+      return;
+    }
+    const listener = this.props[type];
     if (listener) {
       if (typeof listener !== 'function') {
         if (process.env.NODE_ENV !== 'production') {
           console.warn(
-            `Expected \`${propKey}\` listener to be a function, instead got a value of \`${typeof listener}\` type.`,
+            `Expected \`${type}\` listener to be a function, instead got a value of \`${typeof listener}\` type.`,
           );
         }
         return;
@@ -240,6 +254,22 @@ export class ElementInstance extends BaseInstance {
     }
   }
 
+  public stopPropagation(type: string, timeStamp: number | undefined) {
+    if (timeStamp === undefined) {
+      if (process.env.NODE_ENV !== 'production') {
+        console.warn(
+          '`stopPropagation` should be called with a `timeStamp`, otherwise it will stop all events. This might be an internal error in GojiJS.',
+        );
+      }
+      return;
+    }
+    // traverse all ancestors and mark as stopped manually instead of using `e.target.dataset.gojiId` because of this bug:
+    // https://github.com/airbnb/goji-js/issues/198
+    for (let cursor: ElementInstance | undefined = this; cursor; cursor = cursor.parent) {
+      cursor.stoppedPropagation.set(type, timeStamp);
+    }
+  }
+
   public updateProps(newProps: InstanceProps) {
     this.previous = this.pureProps();
     this.props = removeChildrenFromProps(newProps);