Merge pull request #183 from splitio/fix_logs

Update logs for `SplitFactoryProvider` component
splitio · Jan 16, 2024 · d50d900 · d50d900
2 parents 0b869f9 + 045c835
commit d50d900
Show file tree

Hide file tree

Showing 22 changed files with 243 additions and 182 deletions.
diff --git a/.github/workflows/ci.yml → .github/workflows/ci-cd.yml b/.github/workflows/ci.yml → .github/workflows/ci-cd.yml
@@ -23,14 +23,14 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - name: Checkout code
-        uses: actions/checkout@v3
+        uses: actions/checkout@v4
         with:
           fetch-depth: 0
 
       - name: Set up nodejs
         uses: actions/setup-node@v3
         with:
-          node-version: '16.16.0'
+          node-version: 'lts/*'
           cache: 'npm'
 
       - name: npm ci

diff --git a/.nvmrc b/.nvmrc
@@ -1 +1 @@
-v16.16.0
+lts/*
diff --git a/CHANGES.txt b/CHANGES.txt
@@ -1,3 +1,9 @@
+1.11.0 (January 15, 2023)
+ - Added new `SplitFactoryProvider` component as replacement for the now deprecated `SplitFactory` component.
+ This new component is a fixed version of the `SplitFactory` component, which is not handling the SDK initialization side-effects in the `componentDidMount` and `componentDidUpdate` methods (commit phase), causing some issues like the SDK not being reinitialized when component props change (Related to issue #11 and #148).
+ The new component also supports server-side rendering. See our documentation for more details: https://help.split.io/hc/en-us/articles/360038825091-React-SDK#server-side-rendering (Related to issue #11 and #109).
+ - Updated internal code to remove a circular dependency and avoid warning messages with tools like PNPM (Related to issue #176).
+
 1.10.2 (December 12, 2023)
  - Updated @splitsoftware/splitio package to version 10.24.1 that updates localStorage usage to clear cached feature flag definitions before initiating the synchronization process, if the cache was previously synchronized with a different SDK key (i.e., a different environment) or different Split Filter criteria, to avoid using invalid cached data when the SDK is ready from cache.
 

diff --git a/README.md b/README.md
@@ -20,7 +20,7 @@ Below is a simple example that describes the instantiation and most basic usage
 import React from 'react';
 
 // Import SDK functions
-import { SplitFactory, useSplitTreatments } from '@splitsoftware/splitio-react';
+import { SplitFactoryProvider, useSplitTreatments } from '@splitsoftware/splitio-react';
 
 // Define your config object
 const CONFIG = {
@@ -48,10 +48,10 @@ function MyComponent() {
 
 function MyApp() {
   return (
-    // Use SplitFactory to instantiate the SDK and makes it available to nested components
-    <SplitFactory config={CONFIG} >
+    // Use SplitFactoryProvider to instantiate the SDK and makes it available to nested components
+    <SplitFactoryProvider config={CONFIG} >
       <MyComponent />
-    </SplitFactory>
+    </SplitFactoryProvider>
   );
 }
 ```

diff --git a/src/SplitClient.tsx b/src/SplitClient.tsx
@@ -1,13 +1,12 @@
 import React from 'react';
 import { SplitContext } from './SplitContext';
 import { ISplitClientProps, ISplitContextValues, IUpdateProps } from './types';
-import { ERROR_SC_NO_FACTORY } from './constants';
 import { getStatus, getSplitClient, initAttributes, IClientWithContext } from './utils';
 import { DEFAULT_UPDATE_OPTIONS } from './useSplitClient';
 
 /**
  * Common component used to handle the status and events of a Split client passed as prop.
- * Reused by both SplitFactory (main client) and SplitClient (shared client) components.
+ * Reused by both SplitFactoryProvider (main client) and SplitClient (any client) components.
  */
 export class SplitComponent extends React.Component<IUpdateProps & { factory: SplitIO.IBrowserSDK | null, client: SplitIO.IBrowserClient | null, attributes?: SplitIO.Attributes, children: any }, ISplitContextValues> {
 
@@ -47,11 +46,6 @@ export class SplitComponent extends React.Component<IUpdateProps & { factory: Sp
     super(props);
     const { factory, client } = props;
 
-    // Log error if factory is not available
-    if (!factory) {
-      console.error(ERROR_SC_NO_FACTORY);
-    }
-
     this.state = {
       factory,
       client,
@@ -129,9 +123,8 @@ export class SplitComponent extends React.Component<IUpdateProps & { factory: Sp
  * SplitClient will initialize a new SDK client and listen for its events in order to update the Split Context.
  * Children components will have access to the new client when accessing Split Context.
  *
- * Unlike SplitFactory, the underlying SDK client can be changed during the component lifecycle
- * if the component is updated with a different splitKey or trafficType prop. Since the client can change,
- * its release is not handled by SplitClient but by its container SplitFactory component.
+ * The underlying SDK client can be changed during the component lifecycle
+ * if the component is updated with a different splitKey or trafficType prop.
  *
  * @see {@link https://help.split.io/hc/en-us/articles/360020448791-JavaScript-SDK#advanced-instantiate-multiple-sdk-clients}
  */

diff --git a/src/SplitContext.ts b/src/SplitContext.ts
@@ -18,6 +18,6 @@ export const INITIAL_CONTEXT: ISplitContextValues = {
 /**
  * Split Context is the React Context instance that represents our SplitIO global state.
  * It contains Split SDK objects, such as a factory instance, a client and its status (isReady, isTimedout, lastUpdate)
- * The context is created with default empty values, that eventually SplitFactory and SplitClient access and update.
+ * The context is created with default empty values, that SplitFactoryProvider and SplitClient access and update.
  */
 export const SplitContext = React.createContext<ISplitContextValues>(INITIAL_CONTEXT);
diff --git a/src/SplitFactoryProvider.tsx b/src/SplitFactoryProvider.tsx
@@ -3,7 +3,7 @@ import React from 'react';
 import { SplitComponent } from './SplitClient';
 import { ISplitFactoryProps } from './types';
 import { WARN_SF_CONFIG_AND_FACTORY } from './constants';
-import { getSplitFactory, destroySplitFactory, IFactoryWithClients, getSplitClient, getStatus } from './utils';
+import { getSplitFactory, destroySplitFactory, IFactoryWithClients, getSplitClient, getStatus, __factories } from './utils';
 import { DEFAULT_UPDATE_OPTIONS } from './useSplitClient';
 
 /**
@@ -27,24 +27,39 @@ export function SplitFactoryProvider(props: ISplitFactoryProps) {
     config = undefined;
   }
 
-  const [stateFactory, setStateFactory] = React.useState(propFactory || null);
-  const factory = propFactory || stateFactory;
+  const [configFactory, setConfigFactory] = React.useState<IFactoryWithClients | null>(null);
+  const factory = propFactory || (configFactory && config === configFactory.config ? configFactory : null);
   const client = factory ? getSplitClient(factory) : null;
 
+  // Effect to initialize and destroy the factory
   React.useEffect(() => {
     if (config) {
       const factory = getSplitFactory(config);
+
+      return () => {
+        destroySplitFactory(factory);
+      }
+    }
+  }, [config]);
+
+  // Effect to subscribe/unsubscribe to events
+  React.useEffect(() => {
+    const factory = config && __factories.get(config);
+    if (factory) {
       const client = getSplitClient(factory);
       const status = getStatus(client);
 
-      // Update state and unsubscribe from events when first event is emitted
-      const update = () => {
+      // Unsubscribe from events and update state when first event is emitted
+      const update = () => { // eslint-disable-next-line no-use-before-define
+        unsubscribe();
+        setConfigFactory(factory);
+      }
+
+      const unsubscribe = () => {
         client.off(client.Event.SDK_READY, update);
         client.off(client.Event.SDK_READY_FROM_CACHE, update);
         client.off(client.Event.SDK_READY_TIMED_OUT, update);
         client.off(client.Event.SDK_UPDATE, update);
-
-        setStateFactory(factory);
       }
 
       if (updateOnSdkReady) {
@@ -61,10 +76,7 @@ export function SplitFactoryProvider(props: ISplitFactoryProps) {
       }
       if (updateOnSdkUpdate) client.on(client.Event.SDK_UPDATE, update);
 
-      return () => {
-        // Factory destroy unsubscribes from events
-        destroySplitFactory(factory as IFactoryWithClients);
-      }
+      return unsubscribe;
     }
   }, [config, updateOnSdkReady, updateOnSdkReadyFromCache, updateOnSdkTimedout, updateOnSdkUpdate]);
 

diff --git a/src/SplitTreatments.tsx b/src/SplitTreatments.tsx
@@ -1,7 +1,6 @@
 import React from 'react';
 import { SplitContext } from './SplitContext';
 import { ISplitTreatmentsProps, ISplitContextValues } from './types';
-import { WARN_ST_NO_CLIENT } from './constants';
 import { memoizeGetTreatmentsWithConfig } from './utils';
 
 /**
@@ -26,7 +25,7 @@ export class SplitTreatments extends React.Component<ISplitTreatmentsProps> {
         {(splitContext: ISplitContextValues) => {
           const { client, lastUpdate } = splitContext;
           const treatments = this.evaluateFeatureFlags(client, lastUpdate, names, attributes, client ? { ...client.getAttributes() } : {}, flagSets);
-          if (!client) { this.logWarning = true; }
+
           // SplitTreatments only accepts a function as a child, not a React Element (JSX)
           return children({
             ...splitContext, treatments,
@@ -35,9 +34,4 @@ export class SplitTreatments extends React.Component<ISplitTreatmentsProps> {
       </SplitContext.Consumer>
     );
   }
-
-  componentDidMount() {
-    if (this.logWarning) { console.log(WARN_ST_NO_CLIENT); }
-  }
-
 }
diff --git a/src/__tests__/SplitClient.test.tsx b/src/__tests__/SplitClient.test.tsx
@@ -11,17 +11,16 @@ import { sdkBrowser } from './testUtils/sdkConfigs';
 
 /** Test target */
 import { ISplitClientChildProps } from '../types';
-import { SplitFactory } from '../SplitFactory';
+import { SplitFactoryProvider } from '../SplitFactoryProvider';
 import { SplitClient } from '../SplitClient';
 import { SplitContext } from '../SplitContext';
-import { ERROR_SC_NO_FACTORY } from '../constants';
 import { testAttributesBinding, TestComponentProps } from './testUtils/utils';
 
 describe('SplitClient', () => {
 
   test('passes no-ready props to the child if client is not ready.', () => {
     render(
-      <SplitFactory config={sdkBrowser} >
+      <SplitFactoryProvider config={sdkBrowser} >
         <SplitClient splitKey='user1' >
           {({ isReady, isReadyFromCache, hasTimedout, isTimedout, isDestroyed, lastUpdate }: ISplitClientChildProps) => {
             expect(isReady).toBe(false);
@@ -34,7 +33,7 @@ describe('SplitClient', () => {
             return null;
           }}
         </SplitClient>
-      </SplitFactory>
+      </SplitFactoryProvider>
     );
   });
 
@@ -46,7 +45,7 @@ describe('SplitClient', () => {
     await outerFactory.client().ready();
 
     render(
-      <SplitFactory factory={outerFactory} >
+      <SplitFactoryProvider factory={outerFactory} >
         {/* Equivalent to <SplitClient splitKey={undefined} > */}
         <SplitClient splitKey={sdkBrowser.core.key} >
           {({ client, isReady, isReadyFromCache, hasTimedout, isTimedout, isDestroyed, lastUpdate }: ISplitClientChildProps) => {
@@ -61,7 +60,7 @@ describe('SplitClient', () => {
             return null;
           }}
         </SplitClient>
-      </SplitFactory>
+      </SplitFactoryProvider>
     );
   });
 
@@ -76,7 +75,7 @@ describe('SplitClient', () => {
     let previousLastUpdate = -1;
 
     render(
-      <SplitFactory factory={outerFactory} >
+      <SplitFactoryProvider factory={outerFactory} >
         <SplitClient splitKey='user2' updateOnSdkTimedout={true} updateOnSdkUpdate={true} >
           {({ client, isReady, isReadyFromCache, hasTimedout, isTimedout, lastUpdate }: ISplitClientChildProps) => {
             const statusProps = [isReady, isReadyFromCache, hasTimedout, isTimedout];
@@ -106,7 +105,7 @@ describe('SplitClient', () => {
             return null;
           }}
         </SplitClient>
-      </SplitFactory>
+      </SplitFactoryProvider>
     );
 
     act(() => (outerFactory as any).client('user2').__emitter__.emit(Event.SDK_READY_TIMED_OUT));
@@ -128,7 +127,7 @@ describe('SplitClient', () => {
     let previousLastUpdate = -1;
 
     render(
-      <SplitFactory factory={outerFactory} >
+      <SplitFactoryProvider factory={outerFactory} >
         <SplitClient splitKey='user2' updateOnSdkReady={false} updateOnSdkTimedout={true} updateOnSdkUpdate={true} >
           {({ client, isReady, isReadyFromCache, hasTimedout, isTimedout, lastUpdate }: ISplitClientChildProps) => {
             const statusProps = [isReady, isReadyFromCache, hasTimedout, isTimedout];
@@ -152,7 +151,7 @@ describe('SplitClient', () => {
             return null;
           }}
         </SplitClient>
-      </SplitFactory>
+      </SplitFactoryProvider>
     );
 
     act(() => (outerFactory as any).client('user2').__emitter__.emit(Event.SDK_READY_TIMED_OUT));
@@ -172,7 +171,7 @@ describe('SplitClient', () => {
     let previousLastUpdate = -1;
 
     render(
-      <SplitFactory factory={outerFactory} >
+      <SplitFactoryProvider factory={outerFactory} >
         <SplitClient splitKey='user2' >
           {({ client, isReady, isReadyFromCache, hasTimedout, isTimedout, lastUpdate }: ISplitClientChildProps) => {
             const statusProps = [isReady, isReadyFromCache, hasTimedout, isTimedout];
@@ -193,7 +192,7 @@ describe('SplitClient', () => {
             return null;
           }}
         </SplitClient>
-      </SplitFactory>
+      </SplitFactoryProvider>
     );
 
     act(() => (outerFactory as any).client('user2').__emitter__.emit(Event.SDK_READY_TIMED_OUT));
@@ -207,7 +206,7 @@ describe('SplitClient', () => {
     let count = 0;
 
     render(
-      <SplitFactory factory={outerFactory} >
+      <SplitFactoryProvider factory={outerFactory} >
         <SplitClient splitKey='some_user' >
           {({ client }) => {
             count++;
@@ -221,7 +220,7 @@ describe('SplitClient', () => {
             return null;
           }}
         </SplitClient>
-      </SplitFactory>
+      </SplitFactoryProvider>
     );
 
     expect(count).toEqual(2);
@@ -246,26 +245,27 @@ describe('SplitClient', () => {
     };
 
     render(
-      <SplitFactory factory={outerFactory} >
+      <SplitFactoryProvider factory={outerFactory} >
         <SplitClient splitKey='user2' >
           <Component />
         </SplitClient>
-      </SplitFactory>
+      </SplitFactoryProvider>
     );
   });
 
-  test('logs error and passes null client if rendered outside an SplitProvider component.', () => {
-    const errorSpy = jest.spyOn(console, 'error');
-    render(
-      <SplitClient splitKey='user2' >
-        {({ client }) => {
-          expect(client).toBe(null);
-          return null;
-        }}
-      </SplitClient>
-    );
-    expect(errorSpy).toBeCalledWith(ERROR_SC_NO_FACTORY);
-  });
+  // @TODO Update test in breaking change, following common practice in React libraries, like React-redux and React-query: use a falsy value as default context value, and throw an error – instead of logging it – if components are not wrapped in a SplitContext.Provider, i.e., if the context is falsy.
+  // test('logs error and passes null client if rendered outside an SplitProvider component.', () => {
+  //   const errorSpy = jest.spyOn(console, 'error');
+  //   render(
+  //     <SplitClient splitKey='user2' >
+  //       {({ client }) => {
+  //         expect(client).toBe(null);
+  //         return null;
+  //       }}
+  //     </SplitClient>
+  //   );
+  //   expect(errorSpy).toBeCalledWith(ERROR_SC_NO_FACTORY);
+  // });
 
   test(`passes a new client if re-rendered with a different splitKey.
         Only updates the state if the new client triggers an event, but not the previous one.`, (done) => {
@@ -338,24 +338,24 @@ describe('SplitClient', () => {
     }
 
     render(
-      <SplitFactory factory={outerFactory} >
+      <SplitFactoryProvider factory={outerFactory} >
         <InnerComponent />
-      </SplitFactory>
+      </SplitFactoryProvider>
     );
   });
 
   test('attributes binding test with utility', (done) => {
 
     function Component({ attributesFactory, attributesClient, splitKey, testSwitch, factory }: TestComponentProps) {
       return (
-        <SplitFactory factory={factory} attributes={attributesFactory} >
+        <SplitFactoryProvider factory={factory} attributes={attributesFactory} >
           <SplitClient splitKey={splitKey} attributes={attributesClient} trafficType='user' >
             {() => {
               testSwitch(done, splitKey);
               return null;
             }}
           </SplitClient>
-        </SplitFactory>
+        </SplitFactoryProvider>
       );
     }