Merge pull request #219 from splitio/fix_on_update_props

Handle defaults of `updateOnSdk` props, when non boolean values are provided

CHANGES.txt

@@ -1,3 +1,9 @@
+2.0.1 (December 4, 2024)
+ - Updated @splitsoftware/splitio package to version 11.0.3 that includes some improvements and bugfixes.
+ - 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)
  - Added support for targeting rules based on large segments.
  - Added support for passing factory instances to the `factory` prop of the `SplitFactoryProvider` component from other SDK packages that extends the `SplitIO.IBrowserSDK` interface, such as `@splitsoftware/splitio-react-native`, `@splitsoftware/splitio-browserjs` and `@splitsoftware/browser-suite` packages.

package.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"

src/SplitFactoryProvider.tsx

@@ -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/__tests__/SplitClient.test.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__/SplitFactoryProvider.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__/SplitTreatments.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__/useSplitClient.test.tsx

@@ -103,7 +103,7 @@ describe('useSplitClient', () => {
             return null;
           })}
           {React.createElement(() => {
-            const { client, isReady, isReadyFromCache, hasTimedout } = useSplitClient({ splitKey: 'user_2', updateOnSdkUpdate: true });
+            const { client, isReady, isReadyFromCache, hasTimedout } = useSplitClient({ splitKey: 'user_2', updateOnSdkUpdate: undefined /* default is true */ });
             expect(client).toBe(user2Client);
 
             countUseSplitClientUser2++;
@@ -218,7 +218,7 @@ describe('useSplitClient', () => {
     act(() => mainClient.__emitter__.emit(Event.SDK_UPDATE)); // do not trigger re-render because updateOnSdkUpdate is false
     expect(rendersCount).toBe(2);
 
-    wrapper.rerender(<Component updateOnSdkUpdate={true} />); // trigger re-render
+    wrapper.rerender(<Component updateOnSdkUpdate={null /** invalid type should default to `true` */} />); // trigger re-render
     expect(rendersCount).toBe(3);
 
     act(() => mainClient.__emitter__.emit(Event.SDK_UPDATE)); // trigger re-render because updateOnSdkUpdate is true now

src/__tests__/withSplitTreatments.test.tsx

@@ -1,11 +1,12 @@
 import React from 'react';
-import { render } from '@testing-library/react';
+import { act, render } from '@testing-library/react';
 
 /** Mocks */
-import { mockSdk } from './testUtils/mockSplitFactory';
+import { mockSdk, getLastInstance, Event } from './testUtils/mockSplitFactory';
 jest.mock('@splitsoftware/splitio/client', () => {
   return { SplitFactory: mockSdk() };
 });
+import { SplitFactory } from '@splitsoftware/splitio/client';
 import { sdkBrowser } from './testUtils/sdkConfigs';
 
 /** Test target */
@@ -14,12 +15,13 @@ import { withSplitClient } from '../withSplitClient';
 import { withSplitTreatments } from '../withSplitTreatments';
 import { getControlTreatmentsWithConfig } from '../utils';
 
+const featureFlagNames = ['split1', 'split2'];
+
 describe('withSplitTreatments', () => {
 
   it(`passes Split props and outer props to the child.
       In this test, the value of "props.treatments" is obtained by the function "getControlTreatmentsWithConfig",
       and not "client.getTreatmentsWithConfig" since the client is not ready.`, () => {
-    const featureFlagNames = ['split1', 'split2'];
 
     const Component = withSplitFactory(sdkBrowser)<{ outerProp1: string, outerProp2: number }>(
       ({ outerProp1, outerProp2, factory }) => {
@@ -51,4 +53,28 @@ describe('withSplitTreatments', () => {
     render(<Component outerProp1='outerProp1' outerProp2={2} />);
   });
 
+  it('disabling "updateOnSdkTimedout" requires passing `false` in all HOCs since the default value is `true`.', () => {
+
+    let renderCount = 0;
+
+    const Component = withSplitFactory(sdkBrowser)(
+      withSplitClient(sdkBrowser.core.key)(
+        withSplitTreatments(featureFlagNames)(
+          (props) => {
+            renderCount++;
+            expect(props.hasTimedout).toBe(false);
+
+            return null;
+          }, undefined, false
+        ), undefined, false
+      ), undefined, false
+    );
+
+    render(<Component />);
+
+    act(() => getLastInstance(SplitFactory).client().__emitter__.emit(Event.SDK_READY_TIMED_OUT));
+
+    expect(renderCount).toBe(1);
+  });
+
 });

src/useSplitClient.ts

@@ -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,25 +44,29 @@ 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);
 
     // Subscribe to SDK events
-    if (updateOnSdkReady) {
+    if (updateOnSdkReady !== false) {
       if (!statusOnEffect.isReady) client.once(client.Event.SDK_READY, update);
       else if (!status.isReady) update();
     }
-    if (updateOnSdkReadyFromCache) {
+    if (updateOnSdkReadyFromCache !== false) {
       if (!statusOnEffect.isReadyFromCache) client.once(client.Event.SDK_READY_FROM_CACHE, update);
       else if (!status.isReadyFromCache) update();
     }
-    if (updateOnSdkTimedout) {
-      if (!statusOnEffect.hasTimedout) client.once(client.Event.SDK_READY_TIMED_OUT, update);
-      else if (!status.hasTimedout) update();
+    if (updateOnSdkTimedout !== false) {
+      if (!statusOnEffect.hasTimedout) {
+        // Required to avoid error log for event already emitted
+        if (!statusOnEffect.isReady) client.once(client.Event.SDK_READY_TIMED_OUT, update);
+      } else {
+        if (!status.hasTimedout) update();
+      }
     }
-    if (updateOnSdkUpdate) client.on(client.Event.SDK_UPDATE, update);
+    if (updateOnSdkUpdate !== false) client.on(client.Event.SDK_UPDATE, update);
 
     return () => {
       // Unsubscribe from events

src/useTrack.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;
 }