From ec3a644940cd3b77a17e0d8d6e6a1c2ba323b720 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Mon, 4 Nov 2024 14:32:41 -0300 Subject: [PATCH] Refactor SFP to fix an issue with React Strict Mode --- src/SplitFactoryProvider.tsx | 42 +++++++++++++++------ src/__tests__/SplitClient.test.tsx | 4 +- src/__tests__/SplitFactoryProvider.test.tsx | 9 ++--- src/__tests__/SplitTreatments.test.tsx | 4 +- src/useSplitClient.ts | 6 +-- src/useTrack.ts | 2 + src/utils.ts | 29 ++------------ 7 files changed, 46 insertions(+), 50 deletions(-) diff --git a/src/SplitFactoryProvider.tsx b/src/SplitFactoryProvider.tsx index 02e36b0..d9e49b8 100644 --- a/src/SplitFactoryProvider.tsx +++ b/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,12 +29,21 @@ 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]); + // Tomar nota de decisiones en RN: SFP con factory inmediato y sin SC + const factory = React.useMemo 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(() => { @@ -31,15 +52,14 @@ export function SplitFactoryProvider(props: ISplitFactoryProviderProps) { return; } - if (config) { - const factory = getSplitFactory(config); + if (factory) { factory.init && factory.init(); return () => { - destroySplitFactory(factory); + factory.destroy(); } } - }, [config, propFactory]); + }, [config, propFactory, factory]); return ( diff --git a/src/__tests__/SplitClient.test.tsx b/src/__tests__/SplitClient.test.tsx index 70d37e1..4943ffd 100644 --- a/src/__tests__/SplitClient.test.tsx +++ b/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; diff --git a/src/__tests__/SplitFactoryProvider.test.tsx b/src/__tests__/SplitFactoryProvider.test.tsx index bb6c2f5..2a22aea 100644 --- a/src/__tests__/SplitFactoryProvider.test.tsx +++ b/src/__tests__/SplitFactoryProvider.test.tsx @@ -13,7 +13,7 @@ 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'; @@ -54,7 +54,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 +183,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 +196,6 @@ describe('SplitFactoryProvider', () => { {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; })} diff --git a/src/__tests__/SplitTreatments.test.tsx b/src/__tests__/SplitTreatments.test.tsx index b94b477..ce33a96 100644 --- a/src/__tests__/SplitTreatments.test.tsx +++ b/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; }} diff --git a/src/useSplitClient.ts b/src/useSplitClient.ts index 74e26d6..10bd3d9 100644 --- a/src/useSplitClient.ts +++ b/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,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); diff --git a/src/useTrack.ts b/src/useTrack.ts index 2af449b..a71058f 100644 --- a/src/useTrack.ts +++ b/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; } diff --git a/src/utils.ts b/src/utils.ts index f7a8f33..26b5f90 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1,7 +1,6 @@ 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. @@ -9,7 +8,7 @@ import { ISplitStatus } from './types'; /** * 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 = 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 | 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); }