Side effect bindProps should be executed by not excessively memoizing things (#100)

compulim · web-flow · commit a361688e13f2 · 2025-07-26T15:19:00.000-07:00
* Add tests

* Clean up no collision prop

* Add tests

* Simplify

* Add test for side effect

* Add entry

* Fix typing for react@16

* Fix ESLint
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -13,7 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
-- (Preview) 💢 Changed signature to return wrapped return value, instead of plain `ComponentType`, by [@compulim](https://github.com/compulim) in PR [#91](https://github.com/compulim/react-chain-of-responsibility/pull/91), [#92](https://github.com/compulim/react-chain-of-responsibility/pull/92), and [#99](https://github.com/compulim/react-chain-of-responsibility/pull/99)
+- (Preview) 💢 Changed signature to return wrapped return value, instead of plain `ComponentType`, by [@compulim](https://github.com/compulim) in PR [#91](https://github.com/compulim/react-chain-of-responsibility/pull/91), [#92](https://github.com/compulim/react-chain-of-responsibility/pull/92), [#99](https://github.com/compulim/react-chain-of-responsibility/pull/99), and [#100](https://github.com/compulim/react-chain-of-responsibility/pull/100)
 - Use `handler-chain` package, by [@compulim](https://github.com/compulim) in PR [#93](https://github.com/compulim/react-chain-of-responsibility/pull/93)
 - Bumped dependencies, in PR [#97](https://github.com/compulim/react-chain-of-responsibility/pull/97)
   - Development dependencies
diff --git a/packages/react-chain-of-responsibility/src/preview/createChainOfResponsibilityAsRenderCallback.tsx b/packages/react-chain-of-responsibility/src/preview/createChainOfResponsibilityAsRenderCallback.tsx
@@ -145,6 +145,12 @@ type InferProviderProps<T extends InferenceHelper<any, any, any>> = T['~types'][
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
 type InferRequest<T extends InferenceHelper<any, any, any>> = T['~types']['request'];
 
+function createComponentHandlerResult<Props extends BaseProps>(
+  render: (overridingProps?: Partial<Props> | undefined) => ReactNode
+): ComponentHandlerResult<Props> {
+  return Object.freeze({ [DO_NOT_CREATE_THIS_OBJECT_YOURSELF]: undefined, render });
+}
+
 function createChainOfResponsibility<
   Request = void,
   Props extends BaseProps = { readonly children?: never },
@@ -172,21 +178,19 @@ function createChainOfResponsibility<
 
   function reactComponent<P extends Props>(
     component: ComponentType<P>,
+    // For `bindProps` of type function, do not do side-effect in it, it may not be always called in all scenarios.
     bindProps?:
       | (Partial<Props> & Omit<P, keyof Props>)
       | ((props: Props) => Partial<Props> & Omit<P, keyof Props>)
       | undefined
   ): ComponentHandlerResult<Props> {
-    return Object.freeze({
-      [DO_NOT_CREATE_THIS_OBJECT_YOURSELF]: undefined,
-      render: (overridingProps?: Partial<Props> | undefined) => (
-        <ComponentWithProps
-          bindProps={bindProps}
-          component={component as ComponentType<Props>}
-          overridingProps={overridingProps}
-        />
-      )
-    });
+    return createComponentHandlerResult((overridingProps?: Partial<Props> | undefined) => (
+      <ComponentWithProps
+        bindProps={bindProps}
+        component={component as ComponentType<Props>}
+        overridingProps={overridingProps}
+      />
+    ));
   }
 
   const ComponentWithProps = memo(function ComponentWithProps({
@@ -212,8 +216,6 @@ function createChainOfResponsibility<
     return <Component {...props} {...(typeof bindProps === 'function' ? bindProps(props) : bindProps)} />;
   });
 
-  const RENDER_CALLBACK_SYMBOL = `REACT_CHAIN_OF_RESPONSIBILITY:DO_NOT_USE_THIS_RENDER_CALLBACK`;
-
   const useBuildRenderCallback: () => UseBuildRenderCallback<Request, Props> = () => {
     const { enhancer } = useContext(BuildContext);
 
@@ -234,49 +236,35 @@ function createChainOfResponsibility<
               return;
             }
 
-            return Object.freeze({
-              // Convert fallback component as renderer.
-              [DO_NOT_CREATE_THIS_OBJECT_YOURSELF]: undefined,
-              render: () => (
-                // Currently, there are no ways to set `bindProps` to `fallbackComponent`.
-                // `fallbackComponent` do not need `overridingProps` because it is the last one in the chain, it would not have the next() function.
-                <ComponentWithProps component={fallbackComponent} />
-              )
-            });
+            // `fallbackComponent` do not need `overridingProps` because it is the last one in the chain, it would not have the next() function.
+            return reactComponent(fallbackComponent);
           })(request);
 
         return (
           result &&
-          ((props: Props) => (
+          ((originalProps: Props) => (
             // This is render function, we cannot call any hooks here.
-            <RenderCallbackAsComponent
-              {...props} // Spreading the props to leverage React.memo()
-              {...{
-                // TODO: Verify if `result.render` is stable or not, and check performance
-                [RENDER_CALLBACK_SYMBOL]: result.render
-              }}
-            />
+            <BuildRenderCallback originalProps={originalProps} render={result.render} />
           ))
         );
       },
       [enhancer]
     );
   };
 
-  type RenderCallbackAsComponentProps = Props & {
-    // First render function does not need overrideProps.
-    // Override props is for upstreamer to override props before passing to downsteamers.
-    readonly [RENDER_CALLBACK_SYMBOL]: () => ReactNode;
+  type BuildRenderCallbackProps = {
+    readonly originalProps: Props;
+    readonly render: () => ReactNode;
   };
 
-  const RenderCallbackAsComponent = memo(function RenderFunction({
-    [RENDER_CALLBACK_SYMBOL]: render,
-    ...props
-  }: RenderCallbackAsComponentProps) {
-    const context = useMemo<RenderContextType<Props>>(() => Object.freeze({ originalProps: props as Props }), [props]);
+  // Do not memoize <BuildRenderCallback>.
+  // `bindProps` may have side effect and we want to be re-rendered to capture the side-effect.
+  // To prevent wasted render, web devs should memoize it themselves.
+  function BuildRenderCallback({ originalProps, render }: BuildRenderCallbackProps) {
+    const context = useMemo<RenderContextType<Props>>(() => Object.freeze({ originalProps }), [originalProps]);
 
     return <RenderContext.Provider value={context}>{render()}</RenderContext.Provider>;
-  });
+  }
 
   function ChainOfResponsibilityProvider({ children, init, middleware }: ProviderProps<Request, Props, Init>) {
     if (!Array.isArray(middleware) || middleware.some(middleware => typeof middleware !== 'function')) {
@@ -352,7 +340,7 @@ function createChainOfResponsibility<
   return Object.freeze({
     Provider: MemoizedChainOfResponsibilityProvider as typeof MemoizedChainOfResponsibilityProvider &
       InferenceHelper<Request, Props, Init>,
-    Proxy: memo<ProxyProps<Request, Props>>(ChainOfResponsibilityProxy),
+    Proxy: ChainOfResponsibilityProxy,
     reactComponent,
     useBuildRenderCallback
 
diff --git a/packages/react-chain-of-responsibility/src/preview/tests/sideEffect.test.tsx b/packages/react-chain-of-responsibility/src/preview/tests/sideEffect.test.tsx
@@ -0,0 +1,99 @@
+/** @jest-environment jsdom */
+/// <reference types="@types/jest" />
+
+import { scenario } from '@testduet/given-when-then';
+import { render } from '@testing-library/react';
+import React, { Fragment } from 'react';
+
+import createChainOfResponsibility, { type InferMiddleware } from '../createChainOfResponsibilityAsRenderCallback';
+
+type Props = { readonly children?: never };
+type Request = void;
+
+scenario('side effect in bindProps', bdd => {
+  bdd
+    .given('a TestComponent using chain of responsiblity', () => {
+      const { Provider, Proxy, reactComponent, useBuildRenderCallback } = createChainOfResponsibility<Request, Props>();
+
+      let count = 0;
+      const renderCall = jest.fn();
+
+      const MyComponent = function MyComponent({ value }: Props & { readonly value: number }) {
+        renderCall();
+
+        return <Fragment>Hello, World! ({value})</Fragment>;
+      };
+
+      const middleware: readonly InferMiddleware<typeof Provider>[] = [
+        // `bindProps` has side effect.
+        () => () => () => reactComponent(MyComponent, { value: ++count })
+      ];
+
+      return {
+        middleware,
+        MyComponent,
+        Provider,
+        Proxy,
+        renderCall,
+        useBuildRenderCallback
+      };
+    })
+    .and.oneOf([
+      [
+        'rendered using useBuildRenderCallback()',
+        ({ middleware, Provider, renderCall, useBuildRenderCallback }) => {
+          function MyProxy() {
+            const result = useBuildRenderCallback()()?.({});
+
+            return result ? <Fragment>{result}</Fragment> : null;
+          }
+
+          return {
+            renderCall,
+            TestComponent: function TestComponent() {
+              return (
+                <Provider middleware={middleware}>
+                  <MyProxy />
+                </Provider>
+              );
+            }
+          };
+        }
+      ],
+      [
+        'rendered using <Proxy>',
+        ({ middleware, Provider, Proxy, renderCall }) => {
+          return {
+            renderCall,
+            TestComponent: function TestComponent() {
+              return (
+                <Provider middleware={middleware}>
+                  <Proxy request={undefined} />
+                </Provider>
+              );
+            }
+          };
+        }
+      ]
+    ])
+
+    // ---
+
+    .when('the component is rendered', ({ TestComponent }) => render(<TestComponent />))
+    .then('textContent should match', (_, { container }) =>
+      expect(container).toHaveProperty('textContent', 'Hello, World! (1)')
+    )
+    .and('should have rendered once', ({ renderCall }) => expect(renderCall).toHaveBeenCalledTimes(1))
+
+    // ---
+
+    .when('the component is rendered again', ({ TestComponent }, result) => {
+      result.rerender(<TestComponent />);
+
+      return result;
+    })
+    .then('textContent should match', (_, { container }) =>
+      expect(container).toHaveProperty('textContent', 'Hello, World! (2)')
+    )
+    .and('should have rendered twice', ({ renderCall }) => expect(renderCall).toHaveBeenCalledTimes(2));
+});
diff --git a/packages/react-chain-of-responsibility/src/preview/tests/stability.changeFallbackComponent.tsx b/packages/react-chain-of-responsibility/src/preview/tests/stability.changeFallbackComponent.tsx
@@ -0,0 +1,93 @@
+/** @jest-environment jsdom */
+/// <reference types="@types/jest" />
+
+import { scenario } from '@testduet/given-when-then';
+import { render } from '@testing-library/react';
+import React, { Fragment, type ComponentType } from 'react';
+
+import createChainOfResponsibility, { type InferMiddleware } from '../createChainOfResponsibilityAsRenderCallback.tsx';
+
+type Props = { readonly children?: never };
+type Request = number;
+
+scenario('stability test with changing fallbackComponent', bdd => {
+  bdd
+    .given('a TestComponent using chain of responsiblity', () => {
+      const { Provider, useBuildRenderCallback } = createChainOfResponsibility<Request, Props>();
+
+      const alohaCall = jest.fn();
+
+      const Aloha = function MyComponent() {
+        alohaCall();
+
+        return <Fragment>Aloha!</Fragment>;
+      };
+
+      const helloWorldCall = jest.fn();
+
+      const HelloWorld = function MyComponent() {
+        helloWorldCall();
+
+        return <Fragment>Hello, World!</Fragment>;
+      };
+
+      const middleware: readonly InferMiddleware<typeof Provider>[] = [];
+
+      function MyProxy({ fallbackComponent }: { readonly fallbackComponent: ComponentType<Props> }) {
+        // We are using `useBuildRenderCallback` for less memoization.
+        const result = useBuildRenderCallback()(1, { fallbackComponent })?.({});
+
+        return result ? <Fragment>{result}</Fragment> : null;
+      }
+
+      return {
+        Aloha,
+        alohaCall,
+        HelloWorld,
+        helloWorldCall,
+        TestComponent: function TestComponent({
+          fallbackComponent
+        }: {
+          readonly fallbackComponent: ComponentType<Props>;
+        }) {
+          return (
+            <Provider middleware={middleware}>
+              <MyProxy fallbackComponent={fallbackComponent} />
+            </Provider>
+          );
+        }
+      };
+    })
+
+    // ---
+
+    .when('the component is rendered with fallback component of <HelloWorld>', ({ HelloWorld, TestComponent }) =>
+      render(<TestComponent fallbackComponent={HelloWorld} />)
+    )
+    .then('textContent should match', (_, { container }) =>
+      expect(container).toHaveProperty('textContent', 'Hello, World!')
+    )
+    .and('<HelloWorld> should have been rendered once', ({ helloWorldCall }) =>
+      expect(helloWorldCall).toHaveBeenCalledTimes(1)
+    )
+
+    // ---
+
+    .when('the component is rendered again with same fallback component', ({ HelloWorld, TestComponent }) =>
+      render(<TestComponent fallbackComponent={HelloWorld} />)
+    )
+    .then('textContent should match', (_, { container }) =>
+      expect(container).toHaveProperty('textContent', 'Hello, World!')
+    )
+    .and('<HelloWorld> should not have been rendered again', ({ helloWorldCall }) =>
+      expect(helloWorldCall).toHaveBeenCalledTimes(1)
+    )
+
+    // ---
+
+    .when('the component is rendered with fallback component of <Aloha>', ({ Aloha, TestComponent }) =>
+      render(<TestComponent fallbackComponent={Aloha} />)
+    )
+    .then('textContent should match', (_, { container }) => expect(container).toHaveProperty('textContent', 'Aloha!'))
+    .and('<Aloha> should have been rendered once', ({ alohaCall }) => expect(alohaCall).toHaveBeenCalledTimes(1));
+});
diff --git a/packages/react-chain-of-responsibility/src/preview/tests/stability.changeMiddleware.test.tsx b/packages/react-chain-of-responsibility/src/preview/tests/stability.changeMiddleware.test.tsx
diff --git a/packages/react-chain-of-responsibility/src/preview/tests/stability.changeProps.test.tsx b/packages/react-chain-of-responsibility/src/preview/tests/stability.changeProps.test.tsx
