From 1c2d2cf5f7fcbd071b06729b49e796c1788440ee Mon Sep 17 00:00:00 2001
From: Adam Stankiewicz <agstanki@gmail.com>
Date: Fri, 21 Feb 2025 16:15:54 -0500
Subject: [PATCH 1/2] feat: make redux and react-redux truly optional

---
 package-lock.json                        |  8 +++++
 package.json                             |  8 +++++
 src/react/AppProvider.test.jsx           | 38 +++++++++------------
 src/react/OptionalReduxProvider.jsx      | 42 ++++++++++++++++++++++--
 src/react/OptionalReduxProvider.test.jsx | 23 +++++++++++++
 5 files changed, 95 insertions(+), 24 deletions(-)
 create mode 100644 src/react/OptionalReduxProvider.test.jsx

diff --git a/package-lock.json b/package-lock.json
index 18b05366c..bff3cae32 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -62,6 +62,14 @@
         "react-redux": "^7.1.1 || ^8.1.1",
         "react-router-dom": "^6.0.0",
         "redux": "^4.0.4"
+      },
+      "peerDependenciesMeta": {
+        "react-redux": {
+          "optional": true
+        },
+        "redux": {
+          "optional": true
+        }
       }
     },
     "node_modules/@adobe/css-tools": {
diff --git a/package.json b/package.json
index b1c391f58..f454a943f 100644
--- a/package.json
+++ b/package.json
@@ -82,5 +82,13 @@
     "react-redux": "^7.1.1 || ^8.1.1",
     "react-router-dom": "^6.0.0",
     "redux": "^4.0.4"
+  },
+  "peerDependenciesMeta": {
+    "redux": {
+      "optional": true
+    },
+    "react-redux": {
+      "optional": true
+    }
   }
 }
diff --git a/src/react/AppProvider.test.jsx b/src/react/AppProvider.test.jsx
index 9dd05c2c3..985d55888 100644
--- a/src/react/AppProvider.test.jsx
+++ b/src/react/AppProvider.test.jsx
@@ -1,6 +1,6 @@
 import React from 'react';
 import { createStore } from 'redux';
-import { render, waitFor } from '@testing-library/react';
+import { render } from '@testing-library/react';
 import AppProvider from './AppProvider';
 import { initialize } from '../initialize';
 
@@ -48,7 +48,7 @@ describe('AppProvider', () => {
     });
   });
 
-  it('should render its children with a router', async () => {
+  it('should render its children with a router', () => {
     const component = (
       <AppProvider store={createStore(state => state)}>
         <div className="child">Child One</div>
@@ -57,18 +57,16 @@ describe('AppProvider', () => {
     );
 
     const wrapper = render(component);
-    await waitFor(() => {
-      const list = wrapper.container.querySelectorAll('div.child');
-      expect(list.length).toEqual(2);
-      expect(list[0].textContent).toEqual('Child One');
-      expect(list[1].textContent).toEqual('Child Two');
-    });
+    const list = wrapper.container.querySelectorAll('div.child');
+    expect(list.length).toEqual(2);
+    expect(list[0].textContent).toEqual('Child One');
+    expect(list[1].textContent).toEqual('Child Two');
     expect(wrapper.getByTestId('browser-router')).toBeInTheDocument();
     const reduxProvider = wrapper.getByTestId('redux-provider');
     expect(reduxProvider).toBeInTheDocument();
   });
 
-  it('should render its children without a router', async () => {
+  it('should render its children without a router', () => {
     const component = (
       <AppProvider store={createStore(state => state)} wrapWithRouter={false}>
         <div className="child">Child One</div>
@@ -77,18 +75,16 @@ describe('AppProvider', () => {
     );
 
     const wrapper = render(component);
-    await waitFor(() => {
-      const list = wrapper.container.querySelectorAll('div.child');
-      expect(list.length).toEqual(2);
-      expect(list[0].textContent).toEqual('Child One');
-      expect(list[1].textContent).toEqual('Child Two');
-    });
+    const list = wrapper.container.querySelectorAll('div.child');
+    expect(list.length).toEqual(2);
+    expect(list[0].textContent).toEqual('Child One');
+    expect(list[1].textContent).toEqual('Child Two');
     expect(wrapper.queryByTestId('browser-router')).not.toBeInTheDocument();
     const reduxProvider = wrapper.getByTestId('redux-provider');
     expect(reduxProvider).toBeInTheDocument();
   });
 
-  it('should skip redux Provider if not given a store', async () => {
+  it('should skip redux Provider if not given a store', () => {
     const component = (
       <AppProvider>
         <div className="child">Child One</div>
@@ -97,12 +93,10 @@ describe('AppProvider', () => {
     );
 
     const wrapper = render(component);
-    await waitFor(() => {
-      const list = wrapper.container.querySelectorAll('div.child');
-      expect(list.length).toEqual(2);
-      expect(list[0].textContent).toEqual('Child One');
-      expect(list[1].textContent).toEqual('Child Two');
-    });
+    const list = wrapper.container.querySelectorAll('div.child');
+    expect(list.length).toEqual(2);
+    expect(list[0].textContent).toEqual('Child One');
+    expect(list[1].textContent).toEqual('Child Two');
 
     const reduxProvider = wrapper.queryByTestId('redux-provider');
     expect(reduxProvider).not.toBeInTheDocument();
diff --git a/src/react/OptionalReduxProvider.jsx b/src/react/OptionalReduxProvider.jsx
index 8ea0625b8..3458d64fc 100644
--- a/src/react/OptionalReduxProvider.jsx
+++ b/src/react/OptionalReduxProvider.jsx
@@ -1,16 +1,54 @@
-import React from 'react';
+import React, { useEffect, useState } from 'react';
 import PropTypes from 'prop-types';
-import { Provider } from 'react-redux';
+
+function useProvider(store) {
+  const [Provider, setProvider] = useState(null);
+  useEffect(() => {
+    if (!store) {
+      setProvider(() => ({ children: c }) => c);
+      return;
+    }
+    if (process.env.NODE_ENV === 'test') {
+      // In test environments, load react-redux synchronously to avoid async state updates.
+      try {
+        // eslint-disable-next-line global-require
+        const module = require('react-redux');
+        setProvider(() => module.Provider);
+      } catch {
+        setProvider(() => ({ children: c }) => c);
+      }
+    } else {
+      // In production, load react-redux dynamically.
+      import('react-redux')
+        .then((module) => {
+          setProvider(() => module.Provider);
+        })
+        .catch(() => {
+          setProvider(() => ({ children: c }) => c);
+        });
+    }
+  }, [store]);
+  return Provider;
+}
 
 /**
  * @memberof module:React
  * @param {Object} props
  */
 export default function OptionalReduxProvider({ store = null, children }) {
+  const Provider = useProvider(store);
+
+  // If the Provider is not loaded yet, we return null to avoid rendering issues
+  if (!Provider) {
+    return null;
+  }
+
+  // If the store is null, we return the children directly as no Provider is needed
   if (store === null) {
     return children;
   }
 
+  // If the Provider is loaded and the store is not null, we render the Provider with the children
   return (
     <Provider store={store}>
       <div data-testid="redux-provider">
diff --git a/src/react/OptionalReduxProvider.test.jsx b/src/react/OptionalReduxProvider.test.jsx
new file mode 100644
index 000000000..f10169281
--- /dev/null
+++ b/src/react/OptionalReduxProvider.test.jsx
@@ -0,0 +1,23 @@
+import React from 'react';
+import { render, screen } from '@testing-library/react';
+import OptionalReduxProvider from './OptionalReduxProvider'; // Adjust the import path as needed
+
+describe('OptionalReduxProvider', () => {
+  it('should handle error when react-redux import fails', async () => {
+    // Simulate the failed import of 'react-redux'
+    jest.mock('react-redux', () => {
+      throw new Error('Failed to load react-redux');
+    });
+
+    const mockStore = {}; // Mock store object
+    render(
+      <OptionalReduxProvider store={mockStore}>
+        <span>Test Children</span>
+      </OptionalReduxProvider>,
+    );
+
+    // Check that the children are still rendered even when react-redux fails to load
+    const childrenElement = await screen.findByText('Test Children');
+    expect(childrenElement).toBeInTheDocument();
+  });
+});

From 14d12e31825350079d66bb2b609564b3900c4534 Mon Sep 17 00:00:00 2001
From: Adam Stankiewicz <agstanki@gmail.com>
Date: Fri, 21 Feb 2025 17:06:29 -0500
Subject: [PATCH 2/2] chore: updates

---
 src/react/AppProvider.test.jsx      | 31 +++++++++++------------
 src/react/OptionalReduxProvider.jsx | 39 ++++++++++++-----------------
 2 files changed, 30 insertions(+), 40 deletions(-)

diff --git a/src/react/AppProvider.test.jsx b/src/react/AppProvider.test.jsx
index 985d55888..f27fd4476 100644
--- a/src/react/AppProvider.test.jsx
+++ b/src/react/AppProvider.test.jsx
@@ -1,6 +1,6 @@
 import React from 'react';
 import { createStore } from 'redux';
-import { render } from '@testing-library/react';
+import { render, screen, waitFor } from '@testing-library/react';
 import AppProvider from './AppProvider';
 import { initialize } from '../initialize';
 
@@ -48,7 +48,7 @@ describe('AppProvider', () => {
     });
   });
 
-  it('should render its children with a router', () => {
+  it('should render its children with a router', async () => {
     const component = (
       <AppProvider store={createStore(state => state)}>
         <div className="child">Child One</div>
@@ -57,16 +57,16 @@ describe('AppProvider', () => {
     );
 
     const wrapper = render(component);
-    const list = wrapper.container.querySelectorAll('div.child');
-    expect(list.length).toEqual(2);
-    expect(list[0].textContent).toEqual('Child One');
-    expect(list[1].textContent).toEqual('Child Two');
+    await waitFor(() => {
+      expect(screen.getByText('Child One')).toBeInTheDocument();
+      expect(screen.getByText('Child Two')).toBeInTheDocument();
+    });
     expect(wrapper.getByTestId('browser-router')).toBeInTheDocument();
     const reduxProvider = wrapper.getByTestId('redux-provider');
     expect(reduxProvider).toBeInTheDocument();
   });
 
-  it('should render its children without a router', () => {
+  it('should render its children without a router', async () => {
     const component = (
       <AppProvider store={createStore(state => state)} wrapWithRouter={false}>
         <div className="child">Child One</div>
@@ -75,16 +75,16 @@ describe('AppProvider', () => {
     );
 
     const wrapper = render(component);
-    const list = wrapper.container.querySelectorAll('div.child');
-    expect(list.length).toEqual(2);
-    expect(list[0].textContent).toEqual('Child One');
-    expect(list[1].textContent).toEqual('Child Two');
+    await waitFor(() => {
+      expect(screen.getByText('Child One')).toBeInTheDocument();
+      expect(screen.getByText('Child Two')).toBeInTheDocument();
+    });
     expect(wrapper.queryByTestId('browser-router')).not.toBeInTheDocument();
     const reduxProvider = wrapper.getByTestId('redux-provider');
     expect(reduxProvider).toBeInTheDocument();
   });
 
-  it('should skip redux Provider if not given a store', () => {
+  it('should skip redux Provider if not given a store', async () => {
     const component = (
       <AppProvider>
         <div className="child">Child One</div>
@@ -93,11 +93,8 @@ describe('AppProvider', () => {
     );
 
     const wrapper = render(component);
-    const list = wrapper.container.querySelectorAll('div.child');
-    expect(list.length).toEqual(2);
-    expect(list[0].textContent).toEqual('Child One');
-    expect(list[1].textContent).toEqual('Child Two');
-
+    expect(screen.getByText('Child One')).toBeInTheDocument();
+    expect(screen.getByText('Child Two')).toBeInTheDocument();
     const reduxProvider = wrapper.queryByTestId('redux-provider');
     expect(reduxProvider).not.toBeInTheDocument();
   });
diff --git a/src/react/OptionalReduxProvider.jsx b/src/react/OptionalReduxProvider.jsx
index 3458d64fc..792111d39 100644
--- a/src/react/OptionalReduxProvider.jsx
+++ b/src/react/OptionalReduxProvider.jsx
@@ -1,33 +1,29 @@
 import React, { useEffect, useState } from 'react';
 import PropTypes from 'prop-types';
+/* eslint-disable import/no-extraneous-dependencies */
+import { logError } from '@edx/frontend-platform/logging';
+/* eslint-enable import/no-extraneous-dependencies */
 
 function useProvider(store) {
-  const [Provider, setProvider] = useState(null);
+  const [Provider, setProvider] = useState(null); // Initially null to prevent render children that expect a Provider
+
   useEffect(() => {
     if (!store) {
-      setProvider(() => ({ children: c }) => c);
+      setProvider(() => ({ children }) => children); // Ensure fallback if no store
       return;
     }
-    if (process.env.NODE_ENV === 'test') {
-      // In test environments, load react-redux synchronously to avoid async state updates.
+    const loadProvider = async () => {
       try {
-        // eslint-disable-next-line global-require
-        const module = require('react-redux');
-        setProvider(() => module.Provider);
-      } catch {
-        setProvider(() => ({ children: c }) => c);
+        const { Provider: ReactReduxProvider } = await import('react-redux');
+        // Set the Provider from react-redux
+        setProvider(() => ReactReduxProvider);
+      } catch (error) {
+        logError('Failed to load react-redux', error);
       }
-    } else {
-      // In production, load react-redux dynamically.
-      import('react-redux')
-        .then((module) => {
-          setProvider(() => module.Provider);
-        })
-        .catch(() => {
-          setProvider(() => ({ children: c }) => c);
-        });
-    }
+    };
+    loadProvider();
   }, [store]);
+
   return Provider;
 }
 
@@ -38,17 +34,14 @@ function useProvider(store) {
 export default function OptionalReduxProvider({ store = null, children }) {
   const Provider = useProvider(store);
 
-  // If the Provider is not loaded yet, we return null to avoid rendering issues
   if (!Provider) {
     return null;
   }
 
-  // If the store is null, we return the children directly as no Provider is needed
-  if (store === null) {
+  if (!store) {
     return children;
   }
 
-  // If the Provider is loaded and the store is not null, we render the Provider with the children
   return (
     <Provider store={store}>
       <div data-testid="redux-provider">