From c59fe5e22a7b3974b044f930ff10af2bd6f4c4e8 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Thu, 11 Jan 2024 13:54:52 -0300 Subject: [PATCH] Fix logs for new SplitFactoryProvider component --- .github/workflows/ci-cd.yml | 163 +++++++++++++++++++++++++ .nvmrc | 2 +- README.md | 8 +- src/SplitClient.tsx | 13 +- src/SplitTreatments.tsx | 8 +- src/__tests__/SplitClient.test.tsx | 64 +++++----- src/__tests__/SplitTreatments.test.tsx | 63 +++++----- src/constants.ts | 7 +- 8 files changed, 236 insertions(+), 92 deletions(-) create mode 100644 .github/workflows/ci-cd.yml diff --git a/.github/workflows/ci-cd.yml b/.github/workflows/ci-cd.yml new file mode 100644 index 0000000..3870a1f --- /dev/null +++ b/.github/workflows/ci-cd.yml @@ -0,0 +1,163 @@ +name: ci + +on: + push: + branches: + - '**' + pull_request: + branches: + - master + - development + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number }} + cancel-in-progress: true + +permissions: + contents: read + id-token: write + +jobs: + build: + name: Build + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Set up nodejs + uses: actions/setup-node@v3 + with: + node-version: 'lts/*' + cache: 'npm' + + - name: npm ci + run: npm ci + + - name: npm check + run: npm run check + + - name: npm test + run: npm run test -- --coverage + + - name: npm build + run: BUILD_BRANCH=$(echo "${GITHUB_REF#refs/heads/}") npm run build + + - name: Set VERSION env + run: echo "VERSION=$(cat package.json | jq -r .version)" >> $GITHUB_ENV + + - name: SonarQube Scan (Push) + if: github.event_name == 'push' && (github.ref == 'refs/heads/master' || github.ref == 'refs/heads/development') + uses: SonarSource/sonarcloud-github-action@v1.9 + env: + SONAR_TOKEN: ${{ secrets.SONARQUBE_TOKEN }} + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + with: + projectBaseDir: . + args: > + -Dsonar.host.url=${{ secrets.SONARQUBE_HOST }} + -Dsonar.projectVersion=${{ env.VERSION }} + -Dsonar.branch.name=${{ github.ref_name }} + + - name: SonarQube Scan (Pull Request) + if: github.event_name == 'pull_request' + uses: SonarSource/sonarcloud-github-action@v1.9 + env: + SONAR_TOKEN: ${{ secrets.SONARQUBE_TOKEN }} + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + with: + projectBaseDir: . + args: > + -Dsonar.host.url=${{ secrets.SONARQUBE_HOST }} + -Dsonar.projectVersion=${{ env.VERSION }} + -Dsonar.pullrequest.key=${{ github.event.pull_request.number }} + -Dsonar.pullrequest.branch=${{ github.event.pull_request.head.ref }} + -Dsonar.pullrequest.base=${{ github.event.pull_request.base.ref }} + + - name: Store assets + if: github.event_name == 'push' && (github.ref == 'refs/heads/master' || github.ref == 'refs/heads/development') + uses: actions/upload-artifact@v3 + with: + name: assets + path: umd/ + retention-days: 1 + + upload-stage: + name: Upload assets + runs-on: ubuntu-latest + needs: build + if: github.event_name == 'push' && github.ref == 'refs/heads/development' + strategy: + matrix: + environment: + - stage + include: + - environment: stage + account_id: "079419646996" + bucket: split-public-stage + + steps: + - name: Download assets + uses: actions/download-artifact@v3 + with: + name: assets + path: umd + + - name: Display structure of assets + run: ls -R + working-directory: umd + + - name: Configure AWS credentials + uses: aws-actions/configure-aws-credentials@v1-node16 + with: + role-to-assume: arn:aws:iam::${{ matrix.account_id }}:role/gha-public-assets-role + aws-region: us-east-1 + + - name: Upload to S3 + run: aws s3 sync $SOURCE_DIR s3://$BUCKET/$DEST_DIR $ARGS + env: + BUCKET: ${{ matrix.bucket }} + SOURCE_DIR: ./umd + DEST_DIR: sdk + ARGS: --acl public-read --follow-symlinks --cache-control max-age=31536000,public + + upload-prod: + name: Upload assets + runs-on: ubuntu-latest + needs: build + if: github.event_name == 'push' && github.ref == 'refs/heads/master' + strategy: + matrix: + environment: + - prod + include: + - environment: prod + account_id: "825951051969" + bucket: split-public + + steps: + - name: Download assets + uses: actions/download-artifact@v3 + with: + name: assets + path: umd + + - name: Display structure of assets + run: ls -R + working-directory: umd + + - name: Configure AWS credentials + uses: aws-actions/configure-aws-credentials@v1-node16 + with: + role-to-assume: arn:aws:iam::${{ matrix.account_id }}:role/gha-public-assets-role + aws-region: us-east-1 + + - name: Upload to S3 + run: aws s3 sync $SOURCE_DIR s3://$BUCKET/$DEST_DIR $ARGS + env: + BUCKET: ${{ matrix.bucket }} + SOURCE_DIR: ./umd + DEST_DIR: sdk + ARGS: --acl public-read --follow-symlinks --cache-control max-age=31536000,public diff --git a/.nvmrc b/.nvmrc index f274881..b009dfb 100644 --- a/.nvmrc +++ b/.nvmrc @@ -1 +1 @@ -v16.16.0 +lts/* diff --git a/README.md b/README.md index a171ed1..53c4b7f 100644 --- 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 - + // Use SplitFactoryProvider to instantiate the SDK and makes it available to nested components + - + ); } ``` diff --git a/src/SplitClient.tsx b/src/SplitClient.tsx index 0e735a8..fa584f4 100644 --- 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 { @@ -47,11 +46,6 @@ export class SplitComponent extends React.Component { {(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 { ); } - - componentDidMount() { - if (this.logWarning) { console.log(WARN_ST_NO_CLIENT); } - } - } diff --git a/src/__tests__/SplitClient.test.tsx b/src/__tests__/SplitClient.test.tsx index 482403e..3ab262c 100644 --- 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( - + {({ isReady, isReadyFromCache, hasTimedout, isTimedout, isDestroyed, lastUpdate }: ISplitClientChildProps) => { expect(isReady).toBe(false); @@ -34,7 +33,7 @@ describe('SplitClient', () => { return null; }} - + ); }); @@ -46,7 +45,7 @@ describe('SplitClient', () => { await outerFactory.client().ready(); render( - + {/* Equivalent to */} {({ client, isReady, isReadyFromCache, hasTimedout, isTimedout, isDestroyed, lastUpdate }: ISplitClientChildProps) => { @@ -61,7 +60,7 @@ describe('SplitClient', () => { return null; }} - + ); }); @@ -76,7 +75,7 @@ describe('SplitClient', () => { let previousLastUpdate = -1; render( - + {({ client, isReady, isReadyFromCache, hasTimedout, isTimedout, lastUpdate }: ISplitClientChildProps) => { const statusProps = [isReady, isReadyFromCache, hasTimedout, isTimedout]; @@ -106,7 +105,7 @@ describe('SplitClient', () => { return null; }} - + ); act(() => (outerFactory as any).client('user2').__emitter__.emit(Event.SDK_READY_TIMED_OUT)); @@ -128,7 +127,7 @@ describe('SplitClient', () => { let previousLastUpdate = -1; render( - + {({ client, isReady, isReadyFromCache, hasTimedout, isTimedout, lastUpdate }: ISplitClientChildProps) => { const statusProps = [isReady, isReadyFromCache, hasTimedout, isTimedout]; @@ -152,7 +151,7 @@ describe('SplitClient', () => { return null; }} - + ); act(() => (outerFactory as any).client('user2').__emitter__.emit(Event.SDK_READY_TIMED_OUT)); @@ -172,7 +171,7 @@ describe('SplitClient', () => { let previousLastUpdate = -1; render( - + {({ client, isReady, isReadyFromCache, hasTimedout, isTimedout, lastUpdate }: ISplitClientChildProps) => { const statusProps = [isReady, isReadyFromCache, hasTimedout, isTimedout]; @@ -193,7 +192,7 @@ describe('SplitClient', () => { return null; }} - + ); act(() => (outerFactory as any).client('user2').__emitter__.emit(Event.SDK_READY_TIMED_OUT)); @@ -207,7 +206,7 @@ describe('SplitClient', () => { let count = 0; render( - + {({ client }) => { count++; @@ -221,7 +220,7 @@ describe('SplitClient', () => { return null; }} - + ); expect(count).toEqual(2); @@ -246,26 +245,27 @@ describe('SplitClient', () => { }; render( - + - + ); }); - test('logs error and passes null client if rendered outside an SplitProvider component.', () => { - const errorSpy = jest.spyOn(console, 'error'); - render( - - {({ client }) => { - expect(client).toBe(null); - return null; - }} - - ); - 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( + // + // {({ client }) => { + // expect(client).toBe(null); + // return null; + // }} + // + // ); + // 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,9 +338,9 @@ describe('SplitClient', () => { } render( - + - + ); }); @@ -348,14 +348,14 @@ describe('SplitClient', () => { function Component({ attributesFactory, attributesClient, splitKey, testSwitch, factory }: TestComponentProps) { return ( - + {() => { testSwitch(done, splitKey); return null; }} - + ); } diff --git a/src/__tests__/SplitTreatments.test.tsx b/src/__tests__/SplitTreatments.test.tsx index 13d8990..308379c 100644 --- a/src/__tests__/SplitTreatments.test.tsx +++ b/src/__tests__/SplitTreatments.test.tsx @@ -10,13 +10,13 @@ import { SplitFactory as SplitSdk } from '@splitsoftware/splitio/client'; import { sdkBrowser } from './testUtils/sdkConfigs'; import { getStatus } from '../utils'; import { newSplitFactoryLocalhostInstance } from './testUtils/utils'; -import { CONTROL_WITH_CONFIG, WARN_ST_NO_CLIENT } from '../constants'; +import { CONTROL_WITH_CONFIG } from '../constants'; /** Test target */ import { ISplitTreatmentsChildProps, ISplitTreatmentsProps, ISplitClientProps } from '../types'; import { SplitTreatments } from '../SplitTreatments'; import { SplitClient } from '../SplitClient'; -import { SplitFactory } from '../SplitFactory'; +import { SplitFactoryProvider } from '../SplitFactoryProvider'; import { useSplitTreatments } from '../useSplitTreatments'; const logSpy = jest.spyOn(console, 'log'); @@ -28,24 +28,20 @@ describe('SplitTreatments', () => { afterEach(() => { logSpy.mockClear() }); - it('passes control treatments (empty object if flagSets is provided) if the SDK is not ready.', () => { + it('passes control treatments (empty object if flagSets is provided) if the SDK is not operational.', () => { render( - - {({ factory }) => { + + {() => { return ( {({ treatments }: ISplitTreatmentsChildProps) => { - const clientMock: any = factory?.client('user1'); - expect(clientMock.getTreatmentsWithConfig).not.toBeCalled(); expect(treatments).toEqual({ split1: CONTROL_WITH_CONFIG, split2: CONTROL_WITH_CONFIG }); return null; }} {({ treatments }: ISplitTreatmentsChildProps) => { - const clientMock: any = factory?.client('user1'); - expect(clientMock.getTreatmentsWithConfigByFlagSets).not.toBeCalled(); expect(treatments).toEqual({}); return null; }} @@ -53,7 +49,7 @@ describe('SplitTreatments', () => { ); }} - + ); }); @@ -62,7 +58,7 @@ describe('SplitTreatments', () => { (outerFactory as any).client().__emitter__.emit(Event.SDK_READY); render( - + {({ factory, isReady }) => { expect(getStatus(outerFactory.client()).isReady).toBe(isReady); expect(isReady).toBe(true); @@ -90,22 +86,23 @@ describe('SplitTreatments', () => { ); }} - + ); }); - it('logs error and passes control treatments if rendered outside an SplitProvider component.', () => { - render( - - {({ treatments }: ISplitTreatmentsChildProps) => { - expect(treatments).toEqual({ split1: CONTROL_WITH_CONFIG, split2: CONTROL_WITH_CONFIG }); - return null; - }} - - ); + // @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. + // it('logs error and passes control treatments if rendered outside an SplitProvider component.', () => { + // render( + // + // {({ treatments }: ISplitTreatmentsChildProps) => { + // expect(treatments).toEqual({ split1: CONTROL_WITH_CONFIG, split2: CONTROL_WITH_CONFIG }); + // return null; + // }} + // + // ); - expect(logSpy).toBeCalledWith(WARN_ST_NO_CLIENT); - }); + // expect(logSpy).toBeCalledWith(WARN_ST_NO_CLIENT); + // }); /** * Input validation. Passing invalid feature flag names or attributes while the Sdk @@ -113,7 +110,7 @@ describe('SplitTreatments', () => { */ it('Input validation: invalid "names" and "attributes" props in SplitTreatments.', (done) => { render( - + {() => { return ( <> @@ -141,7 +138,7 @@ describe('SplitTreatments', () => { ); }} - + ); expect(logSpy).toBeCalledWith('[ERROR] feature flag names must be a non-empty array.'); expect(logSpy).toBeCalledWith('[ERROR] you passed an invalid feature flag name, feature flag name must be a non-empty string.'); @@ -198,11 +195,11 @@ describe.each([ clientAttributes?: ISplitClientProps['attributes'] }) { return ( - + - + ); } @@ -302,27 +299,27 @@ describe.each([ expect(outerFactory.client('otherKey').getTreatmentsWithConfig).toBeCalledTimes(1); }); - it('rerenders and re-evaluate feature flags when Split context changes (in both SplitFactory and SplitClient components).', async () => { + it('rerenders and re-evaluate feature flags when Split context changes (in both SplitFactoryProvider and SplitClient components).', async () => { // changes in SplitContext implies that either the factory, the client (user key), or its status changed, what might imply a change in treatments const outerFactory = SplitSdk(sdkBrowser); let renderTimesComp1 = 0; let renderTimesComp2 = 0; - // test context updates on SplitFactory + // test context updates on SplitFactoryProvider render( - + {() => { renderTimesComp1++; return null; }} - + ); // test context updates on SplitClient render( - + {() => { @@ -331,7 +328,7 @@ describe.each([ }} - + ); expect(renderTimesComp1).toBe(1); diff --git a/src/constants.ts b/src/constants.ts index 7d81db7..c29d802 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -14,14 +14,11 @@ export const CONTROL_WITH_CONFIG: SplitIO.TreatmentWithConfig = { }; // Warning and error messages -export const WARN_SF_CONFIG_AND_FACTORY: string = '[WARN] Both a config and factory props were provided to SplitFactory. Config prop will be ignored.'; +export const WARN_SF_CONFIG_AND_FACTORY: string = '[WARN] Both a config and factory props were provided to SplitFactoryProvider. Config prop will be ignored.'; +// @TODO remove with SplitFactory component in next major version. SplitFactoryProvider can accept no props and eventually only an initialState export const ERROR_SF_NO_CONFIG_AND_FACTORY: string = '[ERROR] SplitFactory must receive either a Split config or a Split factory as props.'; -export const ERROR_SC_NO_FACTORY: string = '[ERROR] SplitClient does not have access to a Split factory. This is because it is not inside the scope of a SplitFactory component or SplitFactory was not properly instantiated.'; - -export const WARN_ST_NO_CLIENT: string = '[WARN] SplitTreatments does not have access to a Split client. This is because it is not inside the scope of a SplitFactory component or SplitFactory was not properly instantiated.'; - export const EXCEPTION_NO_REACT_OR_CREATECONTEXT: string = 'React library is not available or its version is not supported. Check that it is properly installed or imported. Split SDK requires version 16.3.0+ of React.'; export const WARN_NAMES_AND_FLAGSETS: string = '[WARN] Both names and flagSets properties were provided. flagSets will be ignored.';