Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

45 fix providers to respect subscribed flags #46

Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/asyncWithLDProvider.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -179,4 +179,19 @@ describe('asyncWithLDProvider', () => {
expect(mockInitLDClient).toHaveBeenCalledWith(clientSideID, user, defaultReactOptions, options, flags);
expect(receivedNode).toHaveTextContent('{"devTestFlag":true,"launchDoggly":true}');
});

test('only updates to subscribed flags are pushed to the Provider', async () => {
mockInitLDClient.mockImplementation(() => ({
flags: { testFlag: true },
ldClient: mockLDClient,
}));
mockLDClient.on.mockImplementation((e: string, cb: (c: LDFlagChangeset) => void) => {
cb({ 'test-flag': { current: false, previous: true }, 'another-test-flag': { current: false, previous: true } });
});
const options: LDOptions = {};
const subscribedFlags = { 'test-flag': false };
clayembry marked this conversation as resolved.
Show resolved Hide resolved
const receivedNode = await renderWithConfig({ clientSideID, user, options, flags: subscribedFlags });

expect(receivedNode).toHaveTextContent('{"testFlag":false}');
});
});
13 changes: 4 additions & 9 deletions src/asyncWithLDProvider.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import React, { useState, useEffect, FunctionComponent } from 'react';
import camelCase from 'lodash.camelcase';
import { LDFlagSet, LDFlagChangeset } from 'launchdarkly-js-client-sdk';
import { defaultReactOptions, ProviderConfig } from './types';
import { Provider } from './context';
import initLDClient from './initLDClient';
import { camelCaseKeys } from './utils';
import {camelCaseKeys, getFlattenedFlagsFromChangeset} from './utils';

/**
* This is an async function which initializes LaunchDarkly's JS SDK (`launchdarkly-js-client-sdk`)
Expand Down Expand Up @@ -46,14 +45,10 @@ export default async function asyncWithLDProvider(config: ProviderConfig) {
}

ldClient.on('change', (changes: LDFlagChangeset) => {
const flattened: LDFlagSet = {};
for (const key in changes) {
// tslint:disable-next-line:no-unsafe-any
const flagKey = reactOptions.useCamelCaseFlagKeys ? camelCase(key) : key;
flattened[flagKey] = changes[key].current;
const flattened: LDFlagSet | null = getFlattenedFlagsFromChangeset(changes, flags, reactOptions);
if (flattened) {
setLDData(prev => ({ ...prev, flags: { ...prev.flags, ...flattened } }));
}

setLDData(prev => ({ ...prev, flags: { ...prev.flags, ...flattened } }));
});
}, []);

Expand Down
27 changes: 27 additions & 0 deletions src/provider.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -311,4 +311,31 @@ describe('LDProvider', () => {

expect(mockInitLDClient).toHaveBeenCalledWith(clientSideID, user, defaultReactOptions, options, undefined);
});

test('only updates to subscribed flags are pushed to the Provider', async () => {
mockInitLDClient.mockImplementation(() => ({
flags: { testFlag: true },
ldClient: mockLDClient,
}));
mockLDClient.on.mockImplementation((e: string, cb: (c: LDFlagChangeset) => void) => {
cb({ 'test-flag': { current: false, previous: true }, 'another-test-flag': { current: false, previous: true } });
});
const options: LDOptions = {};
const user: LDUser = { key: 'yus', name: 'yus ng' };
const subscribedFlags = { 'test-flag': false };
const props: ProviderConfig = { clientSideID, user, options, flags: subscribedFlags };
const LaunchDarklyApp = (
<LDProvider {...props}>
<App />
</LDProvider>
);
const instance = create(LaunchDarklyApp).root.findByType(LDProvider).instance as EnhancedComponent;
const mockSetState = jest.spyOn(instance, 'setState');

await instance.componentDidMount();
const callback = mockSetState.mock.calls[1][0] as (flags: LDFlagSet) => LDFlagSet;
const newState = callback({});

expect(newState).toEqual({ flags: { 'testFlag': false } });
});
});
14 changes: 5 additions & 9 deletions src/provider.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import * as React from 'react';
import camelCase from 'lodash.camelcase';
import { LDClient, LDFlagSet, LDFlagChangeset } from 'launchdarkly-js-client-sdk';
import { EnhancedComponent, ProviderConfig, defaultReactOptions } from './types';
import { Provider, LDContext as HocState } from './context';
import initLDClient from './initLDClient';
import { camelCaseKeys } from './utils';
import {camelCaseKeys, getFlattenedFlagsFromChangeset} from './utils';

/**
* The `LDProvider` is a component which accepts a config object which is used to
Expand Down Expand Up @@ -52,15 +51,12 @@ class LDProvider extends React.Component<ProviderConfig, HocState> implements En
getReactOptions = () => ({ ...defaultReactOptions, ...this.props.reactOptions });

subscribeToChanges = (ldClient: LDClient) => {
const { flags } = this.props;
ldClient.on('change', (changes: LDFlagChangeset) => {
const flattened: LDFlagSet = {};
for (const key in changes) {
// tslint:disable-next-line:no-unsafe-any
const { useCamelCaseFlagKeys } = this.getReactOptions();
const flagKey = useCamelCaseFlagKeys ? camelCase(key) : key;
flattened[flagKey] = changes[key].current;
const flattened: LDFlagSet | null = getFlattenedFlagsFromChangeset(changes, flags, this.getReactOptions());
if (flattened) {
this.setState(({flags}) => ({flags: {...flags, ...flattened}}));
}
this.setState(({ flags }) => ({ flags: { ...flags, ...flattened } }));
});
};

Expand Down
59 changes: 58 additions & 1 deletion src/utils.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { camelCaseKeys } from './utils';
import {camelCaseKeys, getFlattenedFlagsFromChangeset} from './utils';
import {LDFlagChangeset, LDFlagSet} from 'launchdarkly-js-client-sdk';
import {LDReactOptions} from './types';

describe('Utils', () => {
test('camelCaseKeys should ignore system keys', () => {
Expand All @@ -15,4 +17,59 @@ describe('Utils', () => {
const result = camelCaseKeys(bootstrap);
expect(result).toEqual({ testFlag: true, anotherTestFlag: false });
});

test('getFlattenedFlagsFromChangeset should return current values of all flags when no targetFlags specified', () => {
const targetFlags: LDFlagSet | undefined = undefined;
const flagChanges: LDFlagChangeset = {
'test-flag': {current: true, previous: false},
'another-test-flag': {current: false, previous: true}
};
const reactOptions: LDReactOptions = {
useCamelCaseFlagKeys: true,
};
const flattened = getFlattenedFlagsFromChangeset(flagChanges, targetFlags, reactOptions);

expect(flattened).toEqual({anotherTestFlag: false, testFlag: true});
})

test('getFlattenedFlagsFromChangeset should return current values only of targetFlags when targetFlags specified', () => {
const targetFlags: LDFlagSet | undefined = {'test-flag': false};
const flagChanges: LDFlagChangeset = {
'test-flag': {current: true, previous: false},
'another-test-flag': {current: false, previous: true}
};
const reactOptions: LDReactOptions = {
useCamelCaseFlagKeys: true,
};
const flattened = getFlattenedFlagsFromChangeset(flagChanges, targetFlags, reactOptions);

expect(flattened).toEqual({testFlag: true});
})

test('getFlattenedFlagsFromChangeset should return null when no targetFlags are changed ', () => {
const targetFlags: LDFlagSet | undefined = {'test-flag': false};
const flagChanges: LDFlagChangeset = {
'another-test-flag': {current: false, previous: true}
};
const reactOptions: LDReactOptions = {
useCamelCaseFlagKeys: true,
};
const flattened = getFlattenedFlagsFromChangeset(flagChanges, targetFlags, reactOptions);

expect(flattened).toBeNull;
})

test('getFlattenedFlagsFromChangeset should not change flags to camelCase when reactOptions.useCamelCaseFlagKeys is false ', () => {
const targetFlags: LDFlagSet | undefined = undefined;
const flagChanges: LDFlagChangeset = {
'test-flag': {current: true, previous: false},
'another-test-flag': {current: false, previous: true}
};
const reactOptions: LDReactOptions = {
useCamelCaseFlagKeys: false,
};
const flattened = getFlattenedFlagsFromChangeset(flagChanges, targetFlags, reactOptions);

expect(flattened).toEqual({'another-test-flag': false, 'test-flag': true});
})
});
28 changes: 26 additions & 2 deletions src/utils.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { LDFlagSet } from 'launchdarkly-js-client-sdk';
import {LDFlagChangeset, LDFlagSet} from 'launchdarkly-js-client-sdk';
import camelCase from 'lodash.camelcase';
import {LDReactOptions} from "./types";

/**
* Transforms a set of flags so that their keys are camelCased. This function ignores
Expand All @@ -20,4 +21,27 @@ export const camelCaseKeys = (rawFlags: LDFlagSet) => {
return flags;
};

export default { camelCaseKeys };
/**
* Gets the flags to pass to the provider from the changeset.
*
* @param changes the `LDFlagChangeset` from the ldClient onchange handler.
* @param targetFlags if targetFlags are specified, changes to other flags are ignored and not returned in the
* flattened `LDFlagSet`
* @param reactOptions reactOptions.useCamelCaseFlagKeys is used to determine whether to
* @return an `LDFlagSet` with the current flag values from the LDFlagChangeset filtered by `targetFlags`
* or null if none of the targetFlags were changed
*/
export const getFlattenedFlagsFromChangeset = (changes: LDFlagChangeset, targetFlags: LDFlagSet | undefined,
reactOptions: LDReactOptions): LDFlagSet | null => {
const flattened: LDFlagSet = {};
for (const key in changes) {
if (targetFlags === undefined || targetFlags[key] !== undefined) {
// tslint:disable-next-line:no-unsafe-any
const flagKey = reactOptions.useCamelCaseFlagKeys ? camelCase(key) : key;
flattened[flagKey] = changes[key].current;
}
}
return Object.keys(flattened).length > 0 ? flattened : null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to return an empty LDFlagSet in the case that no targeted flags are changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, in most cases I would agree as it's typically the case that an empty map or array returned from a function will be used in exactly the same way as a collection with members. But in this case the consumers are going to take a conditional action based on whether the map has keys or not. In this case I thought the contract was more clear by returning null instead of empty map. It also means the consumers don't have to do this Object.keys check themselves.

I'm happy to change it, but just wanted to explain my reasoning.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your reasoning. Part of my reasoning is that this simplifies the utility function and removes the conditional typing from the return type, all while only adding minimal complexity to the consumers. All in all I think what you have in your latest changeset is better, thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your reasoning. Part of my reasoning is that this simplifies the utility function and removes the conditional typing from the return type, all while only adding minimal complexity to the consumers. All in all I think what you have in your latest changeset is better, thanks.

}

export default { camelCaseKeys, getFlattenedFlagsFromChangeset };