Merge pull request #217 from splitio/refactor_for_strict_mode

Refactor for React Strict Mode

Author: EmilianoSanchez

CHANGES.txt

@@ -1,5 +1,6 @@
-2.0.1 (November XX, 2024)
+2.0.1 (December 4, 2024)
  - Updated internal handling of the `updateOnSdkTimedout` param to remove the wrong log "[ERROR] A listener was added for SDK_READY_TIMED_OUT on the SDK, which has already fired and won't be emitted again".
+ - Updated implementation of `SplitFactoryProvider` component to support React Strict Mode (Related to https://github.com/splitio/react-client/issues/221).
  - Bugfixing - Fixed an issue with the `updateOn***` object parameters of the `useSplitClient` and `useSplitTreatments` hooks, and their components and HOCs alternatives, which were not defaulting to `true` when a non-boolean value was provided.
 
 2.0.0 (November 1, 2024)

package-lock.json

@@ -1,6 +1,6 @@
 {
   "name": "@splitsoftware/splitio-react",
-  "version": "2.0.0",
+  "version": "2.0.1",
   "description": "A React library to easily integrate and use Split JS SDK",
   "main": "cjs/index.js",
   "module": "esm/index.js",
@@ -63,7 +63,7 @@
   },
   "homepage": "https://github.com/splitio/react-client#readme",
   "dependencies": {
-    "@splitsoftware/splitio": "11.0.0",
+    "@splitsoftware/splitio": "11.0.3",
     "memoize-one": "^5.1.1",
     "shallowequal": "^1.1.0",
     "tslib": "^2.3.1"

package.json

@@ -1,9 +1,21 @@
 import React from 'react';
 
 import { ISplitFactoryProviderProps } from './types';
-import { WARN_SF_CONFIG_AND_FACTORY } from './constants';
-import { getSplitFactory, destroySplitFactory, getSplitClient, getStatus, initAttributes } from './utils';
+import { VERSION, WARN_SF_CONFIG_AND_FACTORY } from './constants';
+import { getSplitClient, getStatus, initAttributes } from './utils';
 import { SplitContext } from './SplitContext';
+import { SplitFactory } from '@splitsoftware/splitio/client';
+
+/**
+ * Implementation rationale:
+ * - Follows React rules: pure components & hooks, with side effects managed in `useEffect`.
+ * - The `factory` and `client` properties in the context are available from the initial render, rather than being set lazily in a `useEffect`, so that:
+ *   - Hooks retrieve the correct values from the start; for example, `useTrack` accesses the client's `track` method rather than a no-op function (related to https://github.com/splitio/react-client/issues/198).
+ *   - Hooks can support Suspense and Server components where `useEffect` is not called (related to https://github.com/splitio/react-client/issues/192).
+ *   - Re-renders are avoided for child components that do not depend on the factory being ready (e.g., tracking events, updating attributes, or managing consent).
+ * - `SplitFactoryProvider` updates the context only when props change (`config` or `factory`) but not the state (e.g., client status), preventing unnecessary updates to child components and allowing them to control when to update independently.
+ * - For these reasons, and to reduce component tree depth, `SplitFactoryProvider` no longer wraps the child component in a `SplitClient` component and thus does not accept a child as a function or `updateOn` props anymore.
+ */
 
 /**
  * The SplitFactoryProvider is the top level component that provides the Split SDK factory to all child components via the Split Context.
@@ -17,29 +29,37 @@ import { SplitContext } from './SplitContext';
 export function SplitFactoryProvider(props: ISplitFactoryProviderProps) {
   const { config, factory: propFactory, attributes } = props;
 
-  const factory = React.useMemo(() => {
-    const factory = propFactory || (config ? getSplitFactory(config) : undefined);
-    initAttributes(factory && factory.client(), attributes);
-    return factory;
-  }, [config, propFactory, attributes]);
+  const factory = React.useMemo<undefined | SplitIO.IBrowserSDK & { init?: () => void }>(() => {
+    return propFactory ?
+      propFactory :
+      config ?
+        // @ts-expect-error. 2nd param is not part of type definitions. Used to overwrite the SDK version and enable lazy init
+        SplitFactory(config, (modules) => {
+          modules.settings.version = VERSION;
+          modules.lazyInit = true;
+        }) :
+        undefined;
+  }, [config, propFactory]);
+
   const client = factory ? getSplitClient(factory) : undefined;
 
+  initAttributes(client, attributes);
+
   // Effect to initialize and destroy the factory when config is provided
   React.useEffect(() => {
     if (propFactory) {
       if (config) console.log(WARN_SF_CONFIG_AND_FACTORY);
       return;
     }
 
-    if (config) {
-      const factory = getSplitFactory(config);
+    if (factory) {
       factory.init && factory.init();
 
       return () => {
-        destroySplitFactory(factory);
+        factory.destroy();
       }
     }
-  }, [config, propFactory]);
+  }, [config, propFactory, factory]);
 
   return (
     <SplitContext.Provider value={{ factory, client, ...getStatus(client) }} >

src/SplitFactoryProvider.tsx

@@ -15,7 +15,7 @@ import { SplitFactoryProvider } from '../SplitFactoryProvider';
 import { SplitClient } from '../SplitClient';
 import { SplitContext } from '../SplitContext';
 import { INITIAL_STATUS, testAttributesBinding, TestComponentProps } from './testUtils/utils';
-import { IClientWithContext } from '../utils';
+import { getStatus } from '../utils';
 import { EXCEPTION_NO_SFP } from '../constants';
 
 describe('SplitClient', () => {
@@ -56,7 +56,7 @@ describe('SplitClient', () => {
               client: outerFactory.client(),
               isReady: true,
               isReadyFromCache: true,
-              lastUpdate: (outerFactory.client() as IClientWithContext).__getStatus().lastUpdate
+              lastUpdate: getStatus(outerFactory.client()).lastUpdate
             });
 
             return null;

src/__tests__/SplitClient.test.tsx

@@ -13,14 +13,30 @@ const logSpy = jest.spyOn(console, 'log');
 /** Test target */
 import { SplitFactoryProvider } from '../SplitFactoryProvider';
 import { SplitContext, useSplitContext } from '../SplitContext';
-import { __factories, IClientWithContext } from '../utils';
+import { getStatus } from '../utils';
 import { WARN_SF_CONFIG_AND_FACTORY } from '../constants';
 import { INITIAL_STATUS } from './testUtils/utils';
 import { useSplitClient } from '../useSplitClient';
 
 describe('SplitFactoryProvider', () => {
 
-  test('passes no-ready props to the child if initialized with a config.', () => {
+  test('passes no-ready properties, no factory and no client to the context if initialized without a config and factory props.', () => {
+    render(
+      <SplitFactoryProvider >
+        {React.createElement(() => {
+          const context = useSplitContext();
+          expect(context).toEqual({
+            ...INITIAL_STATUS,
+            factory: undefined,
+            client: undefined,
+          });
+          return null;
+        })}
+      </SplitFactoryProvider>
+    );
+  });
+
+  test('passes no-ready properties to the context if initialized with a config.', () => {
     render(
       <SplitFactoryProvider config={sdkBrowser} >
         {React.createElement(() => {
@@ -36,7 +52,7 @@ describe('SplitFactoryProvider', () => {
     );
   });
 
-  test('passes ready props to the child if initialized with a ready factory.', async () => {
+  test('passes ready properties to the context if initialized with a ready factory.', async () => {
     const outerFactory = SplitFactory(sdkBrowser);
     (outerFactory as any).client().__emitter__.emit(Event.SDK_READY_FROM_CACHE);
     (outerFactory as any).client().__emitter__.emit(Event.SDK_READY);
@@ -54,7 +70,7 @@ describe('SplitFactoryProvider', () => {
             client: outerFactory.client(),
             isReady: true,
             isReadyFromCache: true,
-            lastUpdate: (outerFactory.client() as IClientWithContext).__getStatus().lastUpdate
+            lastUpdate: getStatus(outerFactory.client()).lastUpdate
           });
           return null;
         })}
@@ -183,8 +199,7 @@ describe('SplitFactoryProvider', () => {
 
     wrapper.unmount();
 
-    // Created factories are removed from `factories` cache and `destroy` method is called
-    expect(__factories.size).toBe(0);
+    // factory `destroy` methods are called
     expect(createdFactories.size).toBe(2);
     expect(factoryDestroySpies.length).toBe(2);
     factoryDestroySpies.forEach(spy => expect(spy).toBeCalledTimes(1));
@@ -197,8 +212,6 @@ describe('SplitFactoryProvider', () => {
       <SplitFactoryProvider factory={outerFactory}>
         {React.createElement(() => {
           const { factory } = useSplitClient();
-          // if factory is provided as a prop, `factories` cache is not modified
-          expect(__factories.size).toBe(0);
           destroySpy = jest.spyOn(factory!, 'destroy');
           return null;
         })}

src/__tests__/SplitFactoryProvider.test.tsx

@@ -8,7 +8,7 @@ jest.mock('@splitsoftware/splitio/client', () => {
 });
 import { SplitFactory } from '@splitsoftware/splitio/client';
 import { sdkBrowser } from './testUtils/sdkConfigs';
-import { getStatus, IClientWithContext } from '../utils';
+import { getStatus } from '../utils';
 import { newSplitFactoryLocalhostInstance } from './testUtils/utils';
 import { CONTROL_WITH_CONFIG, EXCEPTION_NO_SFP } from '../constants';
 
@@ -73,7 +73,7 @@ describe('SplitTreatments', () => {
                     expect(clientMock.getTreatmentsWithConfig.mock.calls.length).toBe(1);
                     expect(treatments).toBe(clientMock.getTreatmentsWithConfig.mock.results[0].value);
                     expect(featureFlagNames).toBe(clientMock.getTreatmentsWithConfig.mock.calls[0][0]);
-                    expect([isReady2, isReadyFromCache, hasTimedout, isTimedout, isDestroyed, lastUpdate]).toStrictEqual([true, false, false, false, false, (outerFactory.client() as IClientWithContext).__getStatus().lastUpdate]);
+                    expect([isReady2, isReadyFromCache, hasTimedout, isTimedout, isDestroyed, lastUpdate]).toStrictEqual([true, false, false, false, false, getStatus(outerFactory.client()).lastUpdate]);
                     return null;
                   }}
                 </SplitTreatments>

src/__tests__/SplitTreatments.test.tsx

@@ -1,6 +1,6 @@
 import React from 'react';
 import { useSplitContext } from './SplitContext';
-import { getSplitClient, initAttributes, IClientWithContext, getStatus } from './utils';
+import { getSplitClient, initAttributes, getStatus } from './utils';
 import { ISplitContextValues, IUseSplitClientOptions } from './types';
 
 export const DEFAULT_UPDATE_OPTIONS = {
@@ -33,7 +33,7 @@ export function useSplitClient(options?: IUseSplitClientOptions): ISplitContextV
 
   // @TODO Move `getSplitClient` side effects
   // @TODO Once `SplitClient` is removed, which updates the context, simplify next line as `const client = factory ? getSplitClient(factory, splitKey) : undefined;`
-  const client = factory && splitKey ? getSplitClient(factory, splitKey) : contextClient as IClientWithContext;
+  const client = factory && splitKey ? getSplitClient(factory, splitKey) : contextClient;
 
   initAttributes(client, attributes);
 
@@ -44,7 +44,7 @@ export function useSplitClient(options?: IUseSplitClientOptions): ISplitContextV
   React.useEffect(() => {
     if (!client) return;
 
-    const update = () => setLastUpdate(client.__getStatus().lastUpdate);
+    const update = () => setLastUpdate(getStatus(client).lastUpdate);
 
     // Clients are created on the hook's call, so the status may have changed
     const statusOnEffect = getStatus(client);

src/useSplitClient.ts

@@ -13,5 +13,7 @@ const noOpFalse = () => false;
 export function useTrack(splitKey?: SplitIO.SplitKey): SplitIO.IBrowserClient['track'] {
   // All update options are false to avoid re-renders. The track method doesn't need the client to be operational.
   const { client } = useSplitClient({ splitKey, updateOnSdkReady: false, updateOnSdkReadyFromCache: false, updateOnSdkTimedout: false, updateOnSdkUpdate: false });
+
+  // Retrieve the client `track` rather than a bound version of it, as there is no need to bind the function, and can be used as a reactive dependency that only changes if the underlying client changes.
   return client ? client.track : noOpFalse;
 }

src/useTrack.ts

@@ -1,15 +1,14 @@
 import memoizeOne from 'memoize-one';
 import shallowEqual from 'shallowequal';
-import { SplitFactory } from '@splitsoftware/splitio/client';
-import { CONTROL_WITH_CONFIG, VERSION, WARN_NAMES_AND_FLAGSETS } from './constants';
+import { CONTROL_WITH_CONFIG, WARN_NAMES_AND_FLAGSETS } from './constants';
 import { ISplitStatus } from './types';
 
 // Utils used to access singleton instances of Split factories and clients, and to gracefully shutdown all clients together.
 
 /**
  * ClientWithContext interface.
  */
-export interface IClientWithContext extends SplitIO.IBrowserClient {
+interface IClientWithContext extends SplitIO.IBrowserClient {
   __getStatus(): {
     isReady: boolean;
     isReadyFromCache: boolean;
@@ -26,24 +25,6 @@ export interface IFactoryWithLazyInit extends SplitIO.IBrowserSDK {
   init(): void;
 }
 
-// exported for testing purposes
-export const __factories: Map<SplitIO.IBrowserSettings, IFactoryWithLazyInit> = new Map();
-
-// idempotent operation
-export function getSplitFactory(config: SplitIO.IBrowserSettings) {
-  if (!__factories.has(config)) {
-    // SplitFactory is not an idempotent operation
-    // @ts-expect-error. 2nd param is not part of type definitions. Used to overwrite the SDK version
-    const newFactory = SplitFactory(config, (modules) => {
-      modules.settings.version = VERSION;
-      modules.lazyInit = true;
-    }) as IFactoryWithLazyInit;
-    newFactory.config = config;
-    __factories.set(config, newFactory);
-  }
-  return __factories.get(config) as IFactoryWithLazyInit;
-}
-
 // idempotent operation
 export function getSplitClient(factory: SplitIO.IBrowserSDK, key?: SplitIO.SplitKey): IClientWithContext {
   // factory.client is an idempotent operation
@@ -56,11 +37,6 @@ export function getSplitClient(factory: SplitIO.IBrowserSDK, key?: SplitIO.Split
   return client;
 }
 
-export function destroySplitFactory(factory: IFactoryWithLazyInit): Promise<void> | undefined {
-  __factories.delete(factory.config);
-  return factory.destroy();
-}
-
 // Util used to get client status.
 // It might be removed in the future, if the JS SDK extends its public API with a `getStatus` method
 export function getStatus(client?: SplitIO.IBrowserClient): ISplitStatus {
@@ -79,6 +55,7 @@ export function getStatus(client?: SplitIO.IBrowserClient): ISplitStatus {
 /**
  * Manage client attributes binding
  */
+// @TODO should reset attributes rather than set/merge them, to keep SFP and hooks pure.
 export function initAttributes(client?: SplitIO.IBrowserClient, attributes?: SplitIO.Attributes) {
   if (client && attributes) client.setAttributes(attributes);
 }