Add allowPropagateDuringRender (#2)

* Add allowPropagateDuringRender

* Fix tests

Author: compulim

CHANGELOG.md

@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Added
 
 - First public release
+- Added `allowPropagateDuringRender` option as a safety measure to prevent multiple re-render, by [@compulim](https://github.com/compulim) in PR [#2](https://github.com/compulim/use-propagate/pull/2)
 
 ### Changed
 

README.md

@@ -68,6 +68,14 @@ export function createPropagation<T>(): {
 
 When propagating a value via `useContext`, subscribing nodes will be re-rendered. This behavior may not be desirable for events and certain type of scenarios.
 
+### Why I should not call propagate callback function during render-time?
+
+When the propagate callback function is called during rendering, a warning message will be printed and propagation will be stopped.
+
+This is a safety measure to prevent multiple re-render and potential deadlock situation if listeners save the value to a state and trigger another re-render.
+
+If listeners are controlled and would never trigger re-render, you can pass `allowPropagateDuringRender: true` option to ignore this safety measure.
+
 ### How to get response from the listener or wait for the listener to complete?
 
 Modifies the passing value by following the [`FetchEvent.respondWith`](https://developer.mozilla.org/en-US/docs/Web/API/FetchEvent/respondWith) or [`ExtendableEvent.waitUntil`](https://developer.mozilla.org/en-US/docs/Web/API/ExtendableEvent/waitUntil) pattern.
@@ -87,6 +95,8 @@ const MyComponent = () => {
 };
 ```
 
+Please make sure the propagate callback function is not called during render as it could cause multiple re-render and potential deadlock situation.
+
 ## Contributions
 
 Like us? [Star](https://github.com/compulim/use-propagate/stargazers) us.

packages/integration-test/importDefault.test.mjs

@@ -18,29 +18,22 @@ describe('A propagation', () => {
 
   describe('when render initially', () => {
     let listener;
-    let result;
+    let propagate;
 
     beforeEach(() => {
       listener = jest.fn();
 
-      result = renderHook(
-        ({ value }) => {
-          const propagate = usePropagate();
+      renderHook(() => {
+        propagate = usePropagate();
 
-          useListen(listener);
-
-          if (typeof value !== 'undefined') {
-            propagate(value);
-          }
-        },
-        { initialProps: {} }
-      );
+        useListen(listener);
+      });
     });
 
     test('listener should not fire', () => expect(listener).toHaveBeenCalledTimes(0));
 
     describe('when usePropagate() is called', () => {
-      beforeEach(() => result.rerender({ value: 'Hello, World!' }));
+      beforeEach(() => propagate('Hello, World!'));
 
       test('listener should be called once', () => expect(listener).toHaveBeenCalledTimes(1));
       test('listener should have been called with the value', () =>

packages/integration-test/requireDefault.test.cjs

@@ -18,29 +18,22 @@ describe('A propagation', () => {
 
   describe('when render initially', () => {
     let listener;
-    let result;
+    let propagate;
 
     beforeEach(() => {
       listener = jest.fn();
 
-      result = renderHook(
-        ({ value }) => {
-          const propagate = usePropagate();
+      renderHook(() => {
+        propagate = usePropagate();
 
-          useListen(listener);
-
-          if (typeof value !== 'undefined') {
-            propagate(value);
-          }
-        },
-        { initialProps: {} }
-      );
+        useListen(listener);
+      });
     });
 
     test('listener should not fire', () => expect(listener).toHaveBeenCalledTimes(0));
 
     describe('when usePropagate() is called', () => {
-      beforeEach(() => result.rerender({ value: 'Hello, World!' }));
+      beforeEach(() => propagate('Hello, World!'));
 
       test('listener should be called once', () => expect(listener).toHaveBeenCalledTimes(1));
       test('listener should have been called with the value', () =>

packages/integration-test/simple.test.mjs

@@ -1,26 +1,28 @@
 /** @jest-environment jsdom */
 
-import { act, renderHook } from '@testing-library/react';
+import { act } from '@testing-library/react';
 import { createPropagation } from 'use-propagate';
 
+const renderHook =
+  // eslint-disable-next-line @typescript-eslint/no-var-requires
+  require('@testing-library/react').renderHook ||
+  // eslint-disable-next-line @typescript-eslint/no-var-requires
+  require('@testing-library/react-hooks').renderHook;
+
 test('When a value is being propagated, listener should receive the value', () => {
-  const { Provider, useListen, usePropagate } = createPropagation();
+  const { useListen, usePropagate } = createPropagation();
   const listener = jest.fn();
+  let propagate;
 
-  const result = renderHook(
-    ({ value }) => {
-      const propagate = usePropagate();
-
-      useListen(listener);
+  renderHook(() => {
+    propagate = usePropagate();
 
-      typeof value === 'undefined' || propagate(value);
-    },
-    { initialProps: {}, wrapper: Provider }
-  );
+    useListen(listener);
+  });
 
   expect(listener).toHaveBeenCalledTimes(0);
 
-  act(() => result.rerender({ value: 'Hello, World!' }));
+  act(() => propagate('Hello, World!'));
 
   expect(listener).toHaveBeenCalledTimes(1);
   expect(listener).toHaveBeenNthCalledWith(1, 'Hello, World!');

packages/use-propagate/src/createPropagation.spec.tsx

@@ -31,32 +31,25 @@ describe('A propagation', () => {
 
   describe('when render initially', () => {
     let listener: jest.Mock<void, [number]>;
-    let result: RenderHookResult<void, Props>;
+    let propagate: (value: number) => void;
 
     beforeEach(() => {
       listener = jest.fn<void, [number]>();
 
-      result = renderHook<void, Props>(
-        ({ value }) => {
-          const propagate = usePropagate();
+      renderHook<void, Props>(() => {
+        propagate = usePropagate();
 
-          useListen(listener);
-
-          if (typeof value !== 'undefined') {
-            propagate(value);
-          }
-        },
-        { initialProps: {} }
-      );
+        useListen(listener);
+      });
     });
 
     test('listener should not fire', () => expect(listener).toHaveBeenCalledTimes(0));
 
     describe('when usePropagate() is called', () => {
-      beforeEach(() => result.rerender({ value: 1 }));
+      beforeEach(() => act(() => propagate(123)));
 
       test('listener should be called once', () => expect(listener).toHaveBeenCalledTimes(1));
-      test('listener should have been called with the value', () => expect(listener).toHaveBeenNthCalledWith(1, 1));
+      test('listener should have been called with the value', () => expect(listener).toHaveBeenNthCalledWith(1, 123));
     });
   });
 
@@ -139,4 +132,62 @@ describe('A propagation', () => {
       });
     });
   });
+
+  describe('when propagated during render-time', () => {
+    let listener: jest.Mock<void, [number]>;
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    let spy: jest.SpyInstance<void, any[], any>;
+
+    beforeEach(() => {
+      spy = jest.spyOn(console, 'warn').mockImplementation(() => {});
+      listener = jest.fn<void, [number]>();
+
+      renderHook<void, Props>(() => {
+        useListen(listener);
+        usePropagate()(123);
+      });
+    });
+
+    afterEach(() => spy.mockRestore());
+
+    test('should warn', () => {
+      expect(spy).toHaveBeenCalledTimes(1);
+      expect(spy).toHaveBeenNthCalledWith(1, expect.stringMatching(/^use-propagate:\s/u));
+    });
+
+    test('should not call listener', () => expect(listener).toHaveBeenCalledTimes(0));
+  });
+});
+
+describe('A propagation with allowPropagateDuringRender set to true', () => {
+  let useListen: ReturnType<typeof createPropagation<number>>['useListen'];
+  let usePropagate: ReturnType<typeof createPropagation<number>>['usePropagate'];
+
+  beforeEach(() => {
+    ({ useListen, usePropagate } = createPropagation<number>({ allowPropagateDuringRender: true }));
+  });
+
+  describe('when propagated during render-time', () => {
+    let listener: jest.Mock<void, [number]>;
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    let spy: jest.SpyInstance<void, any[], any>;
+
+    beforeEach(() => {
+      spy = jest.spyOn(console, 'warn').mockImplementation(() => {});
+      listener = jest.fn<void, [number]>();
+
+      renderHook<void, Props>(() => {
+        useListen(listener);
+        usePropagate()(123);
+      });
+    });
+
+    afterEach(() => spy.mockRestore());
+
+    test('should not warn', () => expect(spy).toHaveBeenCalledTimes(0));
+    test('should have called the listener', () => {
+      expect(listener).toHaveBeenCalledTimes(1);
+      expect(listener).toHaveBeenNthCalledWith(1, 123);
+    });
+  });
 });

packages/use-propagate/src/createPropagation.tsx

@@ -1,30 +1,53 @@
-import { useEffect } from 'react';
+import { useCallback, useEffect, useLayoutEffect, useMemo } from 'react';
 import { useRefFrom } from 'use-ref-from';
 
-import removeInline from './private/removeInline';
-
 type Listener<T> = (value: T) => void;
 
-export default function createPropagation<T>() {
+type Init = {
+  /**
+   * `true` to allows calling propagate callback function during render-time, otherwise, `false` (default).
+   *
+   * Propagation during render-time is normally discouraged. If listeners save the value into a state, multiple re-render could occur.
+   * This option prevents render deadlock by disallowing propagation during render-time.
+   */
+  allowPropagateDuringRender?: boolean | undefined;
+};
+
+export default function createPropagation<T>(init: Init = {}) {
   type Fn = Listener<T>;
 
-  const listeners: Fn[] = [];
+  let rendering: boolean = false;
+
+  const { allowPropagateDuringRender } = init;
+  const listeners: Set<Fn> = new Set();
+
+  const addListener = (listener: Fn): void => void listeners.add(listener);
+  const removeListener = (listener: Fn): void => void listeners.delete(listener);
+  const usePropagate = () => {
+    rendering = true;
+
+    useLayoutEffect(() => {
+      rendering = false;
+    });
 
-  const addListener = (listener: Fn): void => void listeners.push(listener);
-  const removeListener = (listener: Fn): void => removeInline(listeners, listener);
-  const usePropagate = () => (value: T) => listeners.forEach(listener => listener(value));
+    return (value: T) => {
+      if (rendering && !allowPropagateDuringRender) {
+        return console.warn(
+          'use-propagate: The propagate callback function should not be called while rendering, ignoring the call.'
+        );
+      }
+
+      listeners.forEach(listener => listener(value));
+    };
+  };
 
   return {
     useListen: (listener: Fn) => {
       const listenerRef = useRefFrom(listener);
+      const wrappingListener = useCallback((value: T) => listenerRef.current(value), [listenerRef]);
 
-      useEffect(() => {
-        const wrappingListener = (value: T) => listenerRef.current(value);
-
-        addListener(wrappingListener);
-
-        return () => removeListener(wrappingListener);
-      }, [listenerRef]);
+      useMemo(() => addListener(wrappingListener), [wrappingListener]);
+      useEffect(() => () => removeListener(wrappingListener), [wrappingListener]);
     },
     usePropagate
   };