From 60fe95f2df69368fd384a0bfc235eef3456aafde Mon Sep 17 00:00:00 2001 From: Carson Full Date: Fri, 9 Aug 2019 12:29:38 -0500 Subject: [PATCH 1/9] Change client to be nullable instead of faking default client. This will prevent error if client is not defined and we try to call getTreatmentsWithConfig. Change lastUpdate to just be 0 instead of null for default value. --- src/Split.tsx | 2 +- src/SplitProvider.tsx | 21 +++++++-------------- src/types.ts | 10 +++++----- 3 files changed, 13 insertions(+), 20 deletions(-) diff --git a/src/Split.tsx b/src/Split.tsx index 03c55f2..00f1764 100644 --- a/src/Split.tsx +++ b/src/Split.tsx @@ -16,7 +16,7 @@ const Split: React.SFC = ({ name, children }) => ( {({ client, isReady, lastUpdate }: ISplitContextValues) => children( - isReady + client && isReady ? name instanceof Array ? client.getTreatmentsWithConfig(name as string[]) : client.getTreatmentWithConfig(name as string) diff --git a/src/SplitProvider.tsx b/src/SplitProvider.tsx index e06360b..c836e56 100644 --- a/src/SplitProvider.tsx +++ b/src/SplitProvider.tsx @@ -1,22 +1,15 @@ import { SplitFactory } from '@splitsoftware/splitio'; import React, { createContext } from 'react'; -import { - ISplitContextValues, - ISplitProviderProps, - SplitReactContext, -} from './types'; +import { ISplitContextValues, ISplitProviderProps } from './types'; /** * Creating a React.Context with default values. - * @returns {SplitReactContext} */ -export const SplitContext: SplitReactContext = createContext< - ISplitContextValues ->({ - client: {} as SplitIO.IClient, +export const SplitContext = createContext({ + client: null, isReady: false, - lastUpdate: null, + lastUpdate: 0, }); /** @@ -49,7 +42,7 @@ const SplitProvider = class extends React.Component< this.state = { client: factory.client(), isReady: false, - lastUpdate: null, + lastUpdate: 0, }; } @@ -57,7 +50,7 @@ const SplitProvider = class extends React.Component< * Listening for split events */ componentDidMount() { - const { client } = this.state; + const client = this.state.client!; /** * When SDK is ready this isReady to true @@ -80,7 +73,7 @@ const SplitProvider = class extends React.Component< * Destroying client instance when component unmonts */ componentWillUnmount() { - const { client } = this.state; + const client = this.state.client!; client.destroy(); } diff --git a/src/types.ts b/src/types.ts index 7323a0f..5948137 100644 --- a/src/types.ts +++ b/src/types.ts @@ -17,7 +17,7 @@ export interface ISplitContextValues { * @property {SplitIO.IClient} client * @see {@link https://help.split.io/hc/en-us/articles/360020448791-JavaScript-SDK#2-instantiate-the-sdk-and-create-a-new-split-client} */ - client: SplitIO.IClient; + client: SplitIO.IClient | null; /** * isReady is a property that will show up when the client SDK is ready to be consumed. @@ -28,9 +28,9 @@ export interface ISplitContextValues { /** * Shows up when was the last SDK update - * @property {number | null} lastUpdate + * @property {number} lastUpdate */ - lastUpdate: number | null; + lastUpdate: number; } /** @@ -83,7 +83,7 @@ export interface ISplitProps { */ children: ( treatments: TreatmentWithConfig | TreatmentsWithConfig | null, - client: SplitIO.IClient, - lastUpdate: number | null, + client: SplitIO.IClient | null, + lastUpdate: number, ) => React.ReactNode; } From c2ca531ec5c181121c0396b3b787a276e69657db Mon Sep 17 00:00:00 2001 From: Carson Full Date: Fri, 9 Aug 2019 12:30:41 -0500 Subject: [PATCH 2/9] Convert SplitProvider to function component --- src/SplitProvider.tsx | 100 ++++++++++++++---------------------------- 1 file changed, 34 insertions(+), 66 deletions(-) diff --git a/src/SplitProvider.tsx b/src/SplitProvider.tsx index c836e56..1d661b0 100644 --- a/src/SplitProvider.tsx +++ b/src/SplitProvider.tsx @@ -1,5 +1,5 @@ import { SplitFactory } from '@splitsoftware/splitio'; -import React, { createContext } from 'react'; +import React, { createContext, useEffect, useState } from 'react'; import { ISplitContextValues, ISplitProviderProps } from './types'; @@ -17,7 +17,6 @@ export const SplitContext = createContext({ * Make sure SplitProvider is wrapper your entire app. * @see {@link https://help.split.io/hc/en-us/articles/360020448791-JavaScript-SDK} * @param {ISplitProviderProps} props - * @param {ISplitContextValues} state * @returns {React.Component} * * @example @@ -26,74 +25,43 @@ export const SplitContext = createContext({ * * */ -const SplitProvider = class extends React.Component< - ISplitProviderProps, - ISplitContextValues -> { - constructor(props: ISplitProviderProps) { - super(props); +const SplitProvider = ({ config, children }: ISplitProviderProps) => { + const [client, setClient] = useState(null); + const [isReady, setReady] = useState(false); + const [lastUpdate, setUpdated] = useState(0); - /** - * Instatiating a factory in order to create a client. - * @see {@link https://help.split.io/hc/en-us/articles/360020448791-JavaScript-SDK#2-instantiate-the-sdk-and-create-a-new-split-client} - */ - const factory: SplitIO.ISDK = SplitFactory(props.config); + useEffect(() => { + /** @link https://help.split.io/hc/en-us/articles/360020448791-JavaScript-SDK#2-instantiate-the-sdk-and-create-a-new-split-client */ + const nextClient = SplitFactory(config).client(); + setClient(nextClient); + nextClient.on(nextClient.Event.SDK_READY, () => { + setReady(true); + setUpdated(Date.now()); + }); + nextClient.on(nextClient.Event.SDK_UPDATE, () => { + setUpdated(Date.now()); + }); - this.state = { - client: factory.client(), - isReady: false, - lastUpdate: 0, + return () => { + if (client) { + client.destroy(); + } + setReady(false); + setUpdated(0); }; - } + }, []); - /** - * Listening for split events - */ - componentDidMount() { - const client = this.state.client!; - - /** - * When SDK is ready this isReady to true - */ - client.on(client.Event.SDK_READY, () => this.setState({ isReady: true })); - - /** - * When an update occurs update lastUpdate, this will force a re-render - */ - client.on(client.Event.SDK_UPDATE, () => - this.setState({ - lastUpdate: Date.now(), - }), - ); - - // TODO: Are there any other events that we need to listen? - } - - /** - * Destroying client instance when component unmonts - */ - componentWillUnmount() { - const client = this.state.client!; - - client.destroy(); - } - - render() { - const { isReady, client, lastUpdate } = this.state; - const { children } = this.props; - - return ( - - {children} - - ); - } + return ( + + {children} + + ); }; export default SplitProvider; From 7dfb101edb11caf32fd66cea78d3356804a1cc74 Mon Sep 17 00:00:00 2001 From: Carson Full Date: Fri, 9 Aug 2019 12:31:50 -0500 Subject: [PATCH 3/9] Recreate Client when config changes --- src/SplitProvider.tsx | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/src/SplitProvider.tsx b/src/SplitProvider.tsx index 1d661b0..0ef5d1e 100644 --- a/src/SplitProvider.tsx +++ b/src/SplitProvider.tsx @@ -26,6 +26,38 @@ export const SplitContext = createContext({ * */ const SplitProvider = ({ config, children }: ISplitProviderProps) => { + const { + startup = {}, + scheduler = {}, + core, + features = {}, + storage = {}, + debug, + impressionListener, + } = config; + const clientDeps = [ + startup.readyTimeout, + startup.requestTimeoutBeforeReady, + startup.retriesOnFailureBeforeReady, + startup.eventsFirstPushWindow, + scheduler.featuresRefreshRate, + scheduler.impressionsRefreshRate, + scheduler.metricsRefreshRate, + scheduler.segmentsRefreshRate, + scheduler.eventsPushRate, + scheduler.eventsQueueSize, + scheduler.offlineRefreshRate, + core.authorizationKey, + core.key, + core.trafficType, + core.labelsEnabled, + ...Object.entries(features).map((k, v) => `${k}::${v}`), + storage.type, + storage.prefix, + debug, + impressionListener, + ]; + const [client, setClient] = useState(null); const [isReady, setReady] = useState(false); const [lastUpdate, setUpdated] = useState(0); @@ -49,7 +81,7 @@ const SplitProvider = ({ config, children }: ISplitProviderProps) => { setReady(false); setUpdated(0); }; - }, []); + }, clientDeps); return ( Date: Thu, 22 Aug 2019 11:10:22 -0500 Subject: [PATCH 4/9] Use memoized JSON.stringify to determine whether config has changed. This removes the need for us to know about every single config option, which would be a pain to maintain. --- src/SplitProvider.tsx | 41 +++++++---------------------------------- 1 file changed, 7 insertions(+), 34 deletions(-) diff --git a/src/SplitProvider.tsx b/src/SplitProvider.tsx index 0ef5d1e..a0dc1f0 100644 --- a/src/SplitProvider.tsx +++ b/src/SplitProvider.tsx @@ -1,5 +1,5 @@ import { SplitFactory } from '@splitsoftware/splitio'; -import React, { createContext, useEffect, useState } from 'react'; +import React, { createContext, useEffect, useMemo, useState } from 'react'; import { ISplitContextValues, ISplitProviderProps } from './types'; @@ -26,42 +26,15 @@ export const SplitContext = createContext({ * */ const SplitProvider = ({ config, children }: ISplitProviderProps) => { - const { - startup = {}, - scheduler = {}, - core, - features = {}, - storage = {}, - debug, - impressionListener, - } = config; - const clientDeps = [ - startup.readyTimeout, - startup.requestTimeoutBeforeReady, - startup.retriesOnFailureBeforeReady, - startup.eventsFirstPushWindow, - scheduler.featuresRefreshRate, - scheduler.impressionsRefreshRate, - scheduler.metricsRefreshRate, - scheduler.segmentsRefreshRate, - scheduler.eventsPushRate, - scheduler.eventsQueueSize, - scheduler.offlineRefreshRate, - core.authorizationKey, - core.key, - core.trafficType, - core.labelsEnabled, - ...Object.entries(features).map((k, v) => `${k}::${v}`), - storage.type, - storage.prefix, - debug, - impressionListener, - ]; - const [client, setClient] = useState(null); const [isReady, setReady] = useState(false); const [lastUpdate, setUpdated] = useState(0); + // Determine whether config has changed which would require client to be recreated. + // Convert config object to string so it works with the identity check ran with useEffect's dependency list. + // We memoize this so if the user has memoized their config object we don't call JSON.stringify needlessly. + const configHash = useMemo(() => JSON.stringify(config), [config]); + useEffect(() => { /** @link https://help.split.io/hc/en-us/articles/360020448791-JavaScript-SDK#2-instantiate-the-sdk-and-create-a-new-split-client */ const nextClient = SplitFactory(config).client(); @@ -81,7 +54,7 @@ const SplitProvider = ({ config, children }: ISplitProviderProps) => { setReady(false); setUpdated(0); }; - }, clientDeps); + }, [configHash]); return ( Date: Thu, 22 Aug 2019 11:32:33 -0500 Subject: [PATCH 5/9] Handle impression listener specifically. This allows us to have an onImpression callback property which is idiomatic for React. Also because if the function changed in the config the new function wouldn't be called, because JSON.stringify ignores functions. This is would be confusing for users. Also the fact that we are using JSON.stringify should be transparent, a hidden implementation detail, to users. --- src/SplitProvider.tsx | 35 ++++++++++++++++++++++++++++++++--- src/types.ts | 11 +++++++++-- 2 files changed, 41 insertions(+), 5 deletions(-) diff --git a/src/SplitProvider.tsx b/src/SplitProvider.tsx index a0dc1f0..00d96b0 100644 --- a/src/SplitProvider.tsx +++ b/src/SplitProvider.tsx @@ -1,5 +1,11 @@ import { SplitFactory } from '@splitsoftware/splitio'; -import React, { createContext, useEffect, useMemo, useState } from 'react'; +import React, { + createContext, + useCallback, + useEffect, + useMemo, + useState, +} from 'react'; import { ISplitContextValues, ISplitProviderProps } from './types'; @@ -25,11 +31,29 @@ export const SplitContext = createContext({ * * */ -const SplitProvider = ({ config, children }: ISplitProviderProps) => { +const SplitProvider = ({ + config, + children, + onImpression, +}: ISplitProviderProps) => { const [client, setClient] = useState(null); const [isReady, setReady] = useState(false); const [lastUpdate, setUpdated] = useState(0); + // Handle impression listener separately from config. + // - Allows us to use the onImpression property + // - And because the listener function is ignored from JSON.stringify, + // so changing that in the config wouldn't hook up the new function. + // - Because of this ^ the function can change without us having to recreate the client. + const handleImpression = useCallback((data: SplitIO.ImpressionData) => { + if (onImpression) { + onImpression(data); + } + if (config.impressionListener && config.impressionListener.logImpression) { + config.impressionListener.logImpression(data); + } + }, []); + // Determine whether config has changed which would require client to be recreated. // Convert config object to string so it works with the identity check ran with useEffect's dependency list. // We memoize this so if the user has memoized their config object we don't call JSON.stringify needlessly. @@ -37,7 +61,12 @@ const SplitProvider = ({ config, children }: ISplitProviderProps) => { useEffect(() => { /** @link https://help.split.io/hc/en-us/articles/360020448791-JavaScript-SDK#2-instantiate-the-sdk-and-create-a-new-split-client */ - const nextClient = SplitFactory(config).client(); + const nextClient = SplitFactory({ + ...config, + impressionListener: { + logImpression: handleImpression, + }, + }).client(); setClient(nextClient); nextClient.on(nextClient.Event.SDK_READY, () => { setReady(true); diff --git a/src/types.ts b/src/types.ts index 5948137..90ac864 100644 --- a/src/types.ts +++ b/src/types.ts @@ -41,10 +41,9 @@ export type SplitReactContext = Context; /** * Split Provider interface. Interface that will be implemented in order to create a split provider - * with the SDK browse settings information. The provider will create client out of factory listening + * with the SDK browser settings. The provider will create client out of factory and listen * for SDK events. * @interface ISplitProviderProps - * @see {@link https://docs.split.io/docs/nodejs-sdk-overview#section-listener} */ export interface ISplitProviderProps { /** @@ -54,6 +53,14 @@ export interface ISplitProviderProps { */ config: IBrowserSettings; + /** + * Called when an impression is evaluated. + * This is a convince property that's idiomatic with React. The config option works as well. + * @see {@link https://help.split.io/hc/en-us/articles/360020564931-Node-js-SDK#listener} + * @property {Function} onImpression + */ + onImpression?: SplitIO.IImpressionListener['logImpression']; + /** * Children of our React Split Provider. * @property {React.ReactNode} children From 7d524417d398a8c74ca4fdf9d10451128a9fccf4 Mon Sep 17 00:00:00 2001 From: Carson Full Date: Thu, 22 Aug 2019 11:46:36 -0500 Subject: [PATCH 6/9] Only make state changes if component is mounted --- src/SplitProvider.tsx | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/src/SplitProvider.tsx b/src/SplitProvider.tsx index 00d96b0..9e0eb94 100644 --- a/src/SplitProvider.tsx +++ b/src/SplitProvider.tsx @@ -60,6 +60,14 @@ const SplitProvider = ({ const configHash = useMemo(() => JSON.stringify(config), [config]); useEffect(() => { + // Reset state when re-creating the client after config modification + if (isReady) { + setReady(false); + } + if (lastUpdate > 0) { + setUpdated(0); + } + /** @link https://help.split.io/hc/en-us/articles/360020448791-JavaScript-SDK#2-instantiate-the-sdk-and-create-a-new-split-client */ const nextClient = SplitFactory({ ...config, @@ -68,20 +76,27 @@ const SplitProvider = ({ }, }).client(); setClient(nextClient); + + // Only make state changes if component is mounted. + // https://github.com/facebook/react/issues/14369#issuecomment-468267798 + let isMounted = true; nextClient.on(nextClient.Event.SDK_READY, () => { - setReady(true); - setUpdated(Date.now()); + if (isMounted) { + setReady(true); + setUpdated(Date.now()); + } }); nextClient.on(nextClient.Event.SDK_UPDATE, () => { - setUpdated(Date.now()); + if (isMounted) { + setUpdated(Date.now()); + } }); return () => { + isMounted = false; if (client) { client.destroy(); } - setReady(false); - setUpdated(0); }; }, [configHash]); From 5ede2dfaa1a10ae35799a70568bdbbbe71f816bb Mon Sep 17 00:00:00 2001 From: Carson Full Date: Thu, 22 Aug 2019 12:09:15 -0500 Subject: [PATCH 7/9] Merge ready and update into one state hook since they are updated together. I think this is slightly more performant (one less render call). --- src/SplitProvider.tsx | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/src/SplitProvider.tsx b/src/SplitProvider.tsx index 9e0eb94..2ee5c9c 100644 --- a/src/SplitProvider.tsx +++ b/src/SplitProvider.tsx @@ -37,8 +37,10 @@ const SplitProvider = ({ onImpression, }: ISplitProviderProps) => { const [client, setClient] = useState(null); - const [isReady, setReady] = useState(false); - const [lastUpdate, setUpdated] = useState(0); + const [{ isReady, lastUpdate }, setUpdated] = useState({ + isReady: false, + lastUpdate: 0, + }); // Handle impression listener separately from config. // - Allows us to use the onImpression property @@ -61,11 +63,8 @@ const SplitProvider = ({ useEffect(() => { // Reset state when re-creating the client after config modification - if (isReady) { - setReady(false); - } - if (lastUpdate > 0) { - setUpdated(0); + if (isReady || lastUpdate > 0) { + setUpdated({ isReady: false, lastUpdate: 0 }); } /** @link https://help.split.io/hc/en-us/articles/360020448791-JavaScript-SDK#2-instantiate-the-sdk-and-create-a-new-split-client */ @@ -80,17 +79,13 @@ const SplitProvider = ({ // Only make state changes if component is mounted. // https://github.com/facebook/react/issues/14369#issuecomment-468267798 let isMounted = true; - nextClient.on(nextClient.Event.SDK_READY, () => { - if (isMounted) { - setReady(true); - setUpdated(Date.now()); - } - }); - nextClient.on(nextClient.Event.SDK_UPDATE, () => { + const updateListener = () => { if (isMounted) { - setUpdated(Date.now()); + setUpdated({ isReady: true, lastUpdate: Date.now() }); } - }); + }; + nextClient.on(nextClient.Event.SDK_READY, updateListener); + nextClient.on(nextClient.Event.SDK_UPDATE, updateListener); return () => { isMounted = false; From 995431855bc6f2f6d873144d4308abdfdba0e772 Mon Sep 17 00:00:00 2001 From: Carson Full Date: Thu, 22 Aug 2019 12:49:51 -0500 Subject: [PATCH 8/9] Freeze the config object so users know that modifying it is wrong. Instead they should pass in a new object. --- src/SplitProvider.tsx | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/SplitProvider.tsx b/src/SplitProvider.tsx index 2ee5c9c..894c909 100644 --- a/src/SplitProvider.tsx +++ b/src/SplitProvider.tsx @@ -59,7 +59,10 @@ const SplitProvider = ({ // Determine whether config has changed which would require client to be recreated. // Convert config object to string so it works with the identity check ran with useEffect's dependency list. // We memoize this so if the user has memoized their config object we don't call JSON.stringify needlessly. - const configHash = useMemo(() => JSON.stringify(config), [config]); + // We also freeze the config object here so users know modifying it is not what they want to do. + const configHash = useMemo(() => JSON.stringify(deepFreeze(config)), [ + config, + ]); useEffect(() => { // Reset state when re-creating the client after config modification @@ -109,3 +112,16 @@ const SplitProvider = ({ }; export default SplitProvider; + +const deepFreeze = (object: T): T => { + // Freeze properties before freezing self + const propNames = Object.getOwnPropertyNames(object); + for (const name of propNames) { + const value = object[name]; + if (value && typeof value === 'object') { + object[name] = deepFreeze(value); + } + } + + return Object.freeze(object); +}; From a45a85c4c54dd269481cf901cb59ec0ce02e4a15 Mon Sep 17 00:00:00 2001 From: Carson Full Date: Thu, 22 Aug 2019 13:18:58 -0500 Subject: [PATCH 9/9] Update readme to note performance optimization and onImpression property --- README.md | 46 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 390130a..1ca0a90 100644 --- a/README.md +++ b/README.md @@ -5,6 +5,7 @@ A [Split.io](https://www.split.io/) library to easily manage splits in React. ## Get Started - [Installation](#installation) +- [Configuration](#configuration) - [Usage](#usage) - [Contributions](#install-dependencies) - [All Available Scripts](#all-available-scripts) @@ -24,7 +25,7 @@ samuelcastro@mac:~$ yarn add react-splitio samuelcastro@mac:~$ npm install react-splitio ``` -## Usage +## Configuration On your root component define the Split provider: @@ -34,6 +35,49 @@ On your root component define the Split provider: ``` +### Performance +Note that if your `SDK_CONFIG_OBJECT` is defined inside of a component it will create unnecessary work for `SplitProvider`, +because the object will have a different identity each render (`previousConfig !== newConfig`). + +Instead define config outside of your component: +```tsx +const config = { ... }; + +const Root = () => ( + + + +) +``` +Or if you need to configure dynamically, memoize the object: +```tsx +const MySplitProvider = ({ trafficType, children }) => { + const config = useMemo(() => ({ + core: { + authorizationKey: '', + trafficType, + } + }), [trafficType]); + return ( + + {children} + + ); +}; +``` + +### Impression Listener + +Split allows you to [implement a custom impression listener](https://help.split.io/hc/en-us/articles/360020564931-Node-js-SDK#listener). +`SplitProvider` has an optional convenience `onImpression` callback you can use instead. +```tsx + { + // do something with the impression data. +}}> +``` + +## Usage + Now assuming you have a split named: `feature1` you can do something like: ```jsx