(fix) proxy function props to prevent stale state in callbacks (#604)

* proxy props to prevent callback stale closures

* fix: rollup config for tsx tests

* split tsconfig

* update @playwright/test

.changeset/polite-snakes-enjoy.md

@@ -0,0 +1,5 @@
+---
+"@paypal/react-paypal-js": minor
+---
+
+(fix) proxy props to prevent stale closure

packages/react-paypal-js/.storybook/main.js

@@ -16,7 +16,7 @@ module.exports = {
     typescript: {
         check: false,
         checkOptions: {},
-        reactDocgen: "react-docgen-typescript",
+        reactDocgen: "react-docgen-typescript-plugin",
         reactDocgenTypescriptOptions: {
             // the Storybook docs need this to render the props table for <PayPalButtons />
             compilerOptions: {

packages/react-paypal-js/jest.setup.ts

@@ -0,0 +1 @@
+import "@testing-library/jest-dom";

packages/react-paypal-js/package.json

@@ -66,8 +66,10 @@
         "@storybook/addon-links": "^6.4.9",
         "@storybook/react": "^6.4.9",
         "@storybook/storybook-deployer": "^2.8.10",
+        "@testing-library/jest-dom": "^6.6.3",
         "@testing-library/react": "^12.1.3",
         "@testing-library/react-hooks": "^7.0.2",
+        "@testing-library/user-event": "^14.5.2",
         "@types/jest": "^27.4.0",
         "@types/react": "^17.0.39",
         "@types/react-dom": "^17.0.11",
@@ -84,15 +86,17 @@
         "husky": "^7.0.4",
         "jest": "^27.5.1",
         "jest-mock-extended": "^2.0.4",
-        "react": "^16.14.0",
-        "react-dom": "^16.14.0",
+        "react": "^17.0.2",
+        "react-docgen-typescript-plugin": "^1.0.8",
+        "react-dom": "^17.0.2",
         "react-element-to-jsx-string": "^14.3.4",
         "react-error-boundary": "^3.1.4",
         "react-is": "^17.0.2",
         "rimraf": "^3.0.2",
         "rollup": "^2.67.3",
         "rollup-plugin-cleanup": "^3.2.1",
         "rollup-plugin-terser": "^7.0.2",
+        "scheduler": "^0.20.2",
         "semver": "^7.3.5",
         "standard-version": "^9.3.2",
         "string-template": "^1.0.0",
@@ -105,6 +109,9 @@
     "jest": {
         "transformIgnorePatterns": [
             "/!node_modules\\/@paypal\\/sdk-constants/"
+        ],
+        "setupFilesAfterEnv": [
+            "./jest.setup.ts"
         ]
     },
     "bugs": {

packages/react-paypal-js/rollup.config.js

@@ -9,7 +9,8 @@ import pkg from "./package.json";
 const pkgName = pkg.name.split("@paypal/")[1];
 const banner = getBannerText();
 const tsconfigOverride = {
-    exclude: ["node_modules", "**/*.test.ts"],
+    exclude: ["node_modules", "**/*.test.ts*"],
+    outDir: "./dist",
     target: "es5",
 };
 
@@ -19,6 +20,7 @@ export default [
         input: "src/index.ts",
         plugins: [
             typescript({
+                tsconfig: "./tsconfig.lib.json",
                 ...tsconfigOverride,
             }),
             nodeResolve(),
@@ -56,7 +58,10 @@ export default [
     {
         input: "src/index.ts",
         plugins: [
-            typescript({ ...tsconfigOverride }),
+            typescript({
+                tsconfig: "./tsconfig.lib.json",
+                ...tsconfigOverride,
+            }),
             nodeResolve(),
             cleanup({
                 comments: "none",

packages/react-paypal-js/src/components/PayPalButtons.test.tsx

@@ -6,6 +6,7 @@ import {
     fireEvent,
     act,
 } from "@testing-library/react";
+import userEvent from "@testing-library/user-event";
 import { ErrorBoundary } from "react-error-boundary";
 import { mock } from "jest-mock-extended";
 import { loadScript } from "@paypal/paypal-js";
@@ -450,17 +451,71 @@ describe("<PayPalButtons />", () => {
     test("should accept button message amount as a string", async () => {
         render(
             <PayPalScriptProvider options={{ clientId: "test" }}>
-                <PayPalButtons
-                    message={{ amount: "100" }}
-                />
-            </PayPalScriptProvider>
+                <PayPalButtons message={{ amount: "100" }} />
+            </PayPalScriptProvider>,
         );
 
         await waitFor(() =>
             expect(window.paypal?.Buttons).toHaveBeenCalledWith({
                 message: { amount: "100" },
                 onInit: expect.any(Function),
-            })
+            }),
         );
     });
+
+    test("should not create a stale closure when passing callbacks", async () => {
+        userEvent.setup();
+
+        // @ts-expect-error mocking partial ButtonComponent
+        window.paypal!.Buttons = ({ onClick }: { onClick: () => void }) => ({
+            isEligible: () => true,
+            close: async () => undefined,
+            render: async (ref: HTMLDivElement) => {
+                const el = document.createElement("button");
+                el.id = "mock-button";
+                el.textContent = "Pay";
+                el.addEventListener("click", onClick);
+                ref.appendChild(el);
+            },
+        });
+        const onClickFn = jest.fn();
+
+        const Wrapper = () => {
+            const [count, setCount] = useState(0);
+
+            function onClick() {
+                onClickFn(count);
+            }
+
+            return (
+                <div>
+                    <button
+                        data-testid="count-button"
+                        onClick={() => setCount(count + 1)}
+                    >
+                        Count: {count}
+                    </button>
+                    <PayPalScriptProvider options={{ clientId: "test" }}>
+                        <PayPalButtons onClick={onClick} />
+                    </PayPalScriptProvider>
+                </div>
+            );
+        };
+
+        render(<Wrapper />);
+
+        const countButton = screen.getByTestId("count-button");
+        const payButton = await screen.findByText("Pay");
+        expect(screen.getByText("Count: 0")).toBeInTheDocument();
+
+        await userEvent.click(countButton);
+        expect(await screen.findByText("Count: 1")).toBeInTheDocument();
+        await userEvent.click(payButton);
+        expect(onClickFn).toHaveBeenCalledWith(1);
+
+        await userEvent.click(countButton);
+        expect(await screen.findByText("Count: 2")).toBeInTheDocument();
+        await userEvent.click(payButton);
+        expect(onClickFn).toHaveBeenCalledWith(2);
+    });
 });

packages/react-paypal-js/src/components/PayPalButtons.tsx

@@ -7,6 +7,7 @@ import { SDK_SETTINGS } from "../constants";
 import type { FunctionComponent } from "react";
 import type { PayPalButtonsComponent, OnInitActions } from "@paypal/paypal-js";
 import type { PayPalButtonsComponentProps } from "../types";
+import { useProxyProps } from "../hooks/useProxyProps";
 
 /**
 This `<PayPalButtons />` component supports rendering [buttons](https://developer.paypal.com/docs/business/javascript-sdk/javascript-sdk-reference/#buttons) for PayPal, Venmo, and alternative payment methods.
@@ -25,6 +26,7 @@ export const PayPalButtons: FunctionComponent<PayPalButtonsComponentProps> = ({
     }`.trim();
     const buttonsContainerRef = useRef<HTMLDivElement>(null);
     const buttons = useRef<PayPalButtonsComponent | null>(null);
+    const proxyProps = useProxyProps(buttonProps);
 
     const [{ isResolved, options }] = usePayPalScriptReducer();
     const [initActions, setInitActions] = useState<OnInitActions | null>(null);
@@ -86,7 +88,7 @@ export const PayPalButtons: FunctionComponent<PayPalButtonsComponentProps> = ({
 
         try {
             buttons.current = paypalWindowNamespace.Buttons({
-                ...buttonProps,
+                ...proxyProps,
                 onInit: decoratedOnInit,
             });
         } catch (err) {

packages/react-paypal-js/src/hooks/useProxyProps.test.ts

@@ -0,0 +1,57 @@
+import {
+    CreateOrderActions,
+    CreateOrderData,
+    OnClickActions,
+} from "@paypal/paypal-js";
+import { renderHook } from "@testing-library/react-hooks";
+import { useProxyProps } from "./useProxyProps";
+
+describe("useProxyProps", () => {
+    test("should return an object of wrapped callbacks", () => {
+        const createOrder = jest.fn().mockReturnValue("createOrder");
+        const onClick = jest.fn().mockReturnValue("onClick");
+
+        const props = {
+            createOrder,
+            onClick,
+        };
+
+        const {
+            result: { current },
+        } = renderHook(() => useProxyProps(props));
+
+        expect(current).toHaveProperty("createOrder");
+        expect(current).toHaveProperty("onClick");
+        expect(current.createOrder).not.toBe(props.createOrder);
+        expect(current.onClick).not.toBe(props.onClick);
+
+        expect(
+            current.createOrder!(
+                {} as CreateOrderData,
+                {} as CreateOrderActions,
+            ),
+        ).toBe("createOrder");
+        expect(current.onClick!({}, {} as OnClickActions)).toBe("onClick");
+
+        expect(props.createOrder).toHaveBeenCalled();
+        expect(props.onClick).toHaveBeenCalled();
+
+        // ensure no props mutation
+        expect(props.createOrder).toBe(createOrder);
+        expect(props.onClick).toBe(onClick);
+    });
+
+    test("should not wrap or mutate non-function props", () => {
+        const fundingSource = ["paypal"];
+        const props = {
+            fundingSource,
+        };
+
+        const {
+            result: { current },
+        } = renderHook(() => useProxyProps(props));
+
+        expect(current.fundingSource).toBe(props.fundingSource);
+        expect(props.fundingSource).toBe(fundingSource);
+    });
+});

packages/react-paypal-js/src/hooks/useProxyProps.ts

@@ -0,0 +1,31 @@
+import { useRef } from "react";
+
+export function useProxyProps<T extends Record<PropertyKey, unknown>>(
+    props: T,
+): T {
+    const proxyRef = useRef(
+        new Proxy<T>({} as T, {
+            get(target: T, prop: PropertyKey, receiver) {
+                /**
+                 *
+                 * If target[prop] is a function, return a function that accesses
+                 * this function off the target object. We can mutate the target with
+                 * new copies of this function without having to re-render the
+                 * SDK components to pass new callbacks.
+                 *
+                 * */
+                if (typeof target[prop] === "function") {
+                    return (...args: unknown[]) =>
+                        // eslint-disable-next-line @typescript-eslint/ban-types
+                        (target[prop] as Function)(...args);
+                }
+
+                return Reflect.get(target, prop, receiver);
+            },
+        }),
+    );
+
+    proxyRef.current = Object.assign(proxyRef.current, props);
+
+    return proxyRef.current;
+}

packages/react-paypal-js/src/stories/payPalButtons/PayPalButtons.stories.tsx

@@ -1,4 +1,4 @@
-import React, { useEffect } from "react";
+import React, { useEffect, useState } from "react";
 import { action } from "@storybook/addon-actions";
 
 import { usePayPalScriptReducer, DISPATCH_ACTION } from "../../index";
@@ -256,3 +256,58 @@ export const Donate: FC<Omit<StoryProps, "showSpinner" | "fundingSource">> = ({
     fundingSource: { control: false },
     showSpinner: { table: { disable: true } },
 };
+
+export const WithDynamicOrderState: FC<StoryProps> = ({
+    style,
+    message,
+    fundingSource,
+    disabled,
+    showSpinner,
+}) => {
+    const [count, setCount] = useState(0);
+    const [{ options }, dispatch] = usePayPalScriptReducer();
+    useEffect(() => {
+        dispatch({
+            type: DISPATCH_ACTION.RESET_OPTIONS,
+            value: {
+                ...options,
+                "data-order-id": Date.now().toString(),
+            },
+        });
+        // eslint-disable-next-line react-hooks/exhaustive-deps
+    }, [showSpinner]);
+
+    return (
+        <>
+            {showSpinner && <LoadingSpinner />}
+            <button onClick={() => setCount(count + 1)}>Count: {count}</button>
+            <PayPalButtons
+                style={style}
+                message={message}
+                disabled={disabled}
+                fundingSource={fundingSource}
+                forceReRender={[style]}
+                createOrder={() =>
+                    createOrder([{ sku: "1blwyeo8", quantity: count }]).then(
+                        (orderData) => {
+                            if (orderData.id) {
+                                action(ORDER_ID)(orderData.id);
+                                return orderData.id;
+                            } else {
+                                throw new Error("failed to create Order Id");
+                            }
+                        },
+                    )
+                }
+                onApprove={(data) =>
+                    onApprove(data).then((orderData) =>
+                        action(APPROVE)(orderData),
+                    )
+                }
+                {...defaultProps}
+            >
+                <InEligibleError />
+            </PayPalButtons>
+        </>
+    );
+};

packages/react-paypal-js/tsconfig.json

@@ -9,11 +9,10 @@
         "moduleResolution": "node",
         "target": "esnext",
         "strict": true,
-        "skipLibCheck": true,
-
-        // these are overridden by the Rollup plugin
-        "outDir": "./dist",
-        "rootDir": "./src"
+        "skipLibCheck": true
     },
-    "include": ["src"]
+    "references": [
+        { "path": "./tsconfig.lib.json" },
+        { "path": "./tsconfig.test.json" }
+    ]
 }

packages/react-paypal-js/tsconfig.lib.json

@@ -0,0 +1,8 @@
+{
+    "extends": "./tsconfig.json",
+    // these are overridden by the Rollup plugin
+    "outDir": "./dist",
+    "rootDir": "./src",
+    "include": ["src/**/*.ts", "src/**/*.tsx"],
+    "exclude": ["src/**/*.test.ts", "src/**/*.test.tsx"]
+}

packages/react-paypal-js/tsconfig.test.json

@@ -0,0 +1,4 @@
+{
+    "extends": "./tsconfig.json",
+    "include": ["src/**/*.test.ts", "src/**/*.test.tsx", "jest.setup.ts"]
+}