Skip to content

Commit

Permalink
Refactor getRemoteFeatureFlags to updateRemoteFeatureFlags, such that…
Browse files Browse the repository at this point in the history 
… it no longer returns feature flags
  • Loading branch information
@danjm
danjm committed Nov 28, 2024
1 parent 21aa60b commit 0f09ed5
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 83 deletions.
119 changes: 53 additions & 66 deletions packages/remote-feature-flag-controller/src/remote-feature-flag-controller.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,8 @@ describe('RemoteFeatureFlagController', () => {
});
});

describe('getRemoteFeatureFlags', () => {
it('should return cached data when controller is disabled', async () => {
describe('updateRemoteFeatureFlags', () => {
it('should not make a network request and not modify state if disabled is true', async () => {
const clientConfigApiService = buildClientConfigApiService();
const controller = createController({
state: {
Expand All @@ -95,15 +95,15 @@ describe('RemoteFeatureFlagController', () => {
disabled: true,
});

const remoteFeatureFlags = await controller.getRemoteFeatureFlags();
await controller.updateRemoteFeatureFlags();

expect(remoteFeatureFlags).toStrictEqual(MOCK_FLAGS);
expect(controller.state.remoteFeatureFlags).toStrictEqual(MOCK_FLAGS);
expect(
clientConfigApiService.fetchRemoteFeatureFlags,
).not.toHaveBeenCalled();
});

it('should not make network request to fetch, and should returned cached data, when cache is not expired', async () => {
it('should not make a network request and not modify state when cache is not expired', async () => {
const clientConfigApiService = buildClientConfigApiService();
const controller = createController({
state: {
Expand All @@ -112,12 +112,12 @@ describe('RemoteFeatureFlagController', () => {
},
clientConfigApiService,
});
const remoteFeatureFlags = await controller.getRemoteFeatureFlags();
await controller.updateRemoteFeatureFlags();

expect(
clientConfigApiService.fetchRemoteFeatureFlags,
).not.toHaveBeenCalled();
expect(remoteFeatureFlags).toStrictEqual(MOCK_FLAGS);
expect(controller.state.remoteFeatureFlags).toStrictEqual(MOCK_FLAGS);
});

it('should make network request to fetch when cache is expired, and then update cache', async () => {
Expand All @@ -141,8 +141,7 @@ describe('RemoteFeatureFlagController', () => {
cacheTimestamp: Date.now(),
}));

const remoteFeatureFlags = await controller.getRemoteFeatureFlags();
expect(remoteFeatureFlags).toStrictEqual(MOCK_FLAGS_TWO_WITH_NAMES);
await controller.updateRemoteFeatureFlags();
expect(
clientConfigApiService.fetchRemoteFeatureFlags,
).toHaveBeenCalledTimes(1);
Expand All @@ -155,11 +154,12 @@ describe('RemoteFeatureFlagController', () => {
const clientConfigApiService = buildClientConfigApiService();
const controller = createController({
clientConfigApiService,
state: {
remoteFeatureFlags: MOCK_FLAGS,
cacheTimestamp: Date.now() - 10,
},
});

// First call to set cache
await controller.getRemoteFeatureFlags();

// Mock different response
jest
.spyOn(clientConfigApiService, 'fetchRemoteFeatureFlags')
Expand All @@ -169,29 +169,30 @@ describe('RemoteFeatureFlagController', () => {
}).fetchRemoteFeatureFlags(),
);

const remoteFeatureFlags = await controller.getRemoteFeatureFlags();
await controller.updateRemoteFeatureFlags();

expect(remoteFeatureFlags).toStrictEqual(MOCK_FLAGS_WITH_NAMES);
expect(controller.state.remoteFeatureFlags).toStrictEqual(MOCK_FLAGS);
});

it('should handle concurrent flag updates', async () => {
it('should only make one network request, and only one state update, when there are concurrent calls', async () => {
const clientConfigApiService = buildClientConfigApiService();
const controller = createController({ clientConfigApiService });

const [result1, result2] = await Promise.all([
controller.getRemoteFeatureFlags(),
controller.getRemoteFeatureFlags(),
await Promise.all([
controller.updateRemoteFeatureFlags(),
controller.updateRemoteFeatureFlags(),
]);

expect(result1).toStrictEqual(MOCK_FLAGS_WITH_NAMES);
expect(result2).toStrictEqual(MOCK_FLAGS_WITH_NAMES);
expect(
clientConfigApiService.fetchRemoteFeatureFlags,
).toHaveBeenCalledTimes(1);

expect(controller.state.remoteFeatureFlags).toStrictEqual(
MOCK_FLAGS_WITH_NAMES,
);
});

it('should create a new fetch when called sequentially with awaiting and sufficient delay', async () => {
it('should create a new fetch, and correctly update state, when called sequentially with awaiting and sufficient delay', async () => {
jest.useFakeTimers();
const initialTime = Date.now();
const fetchSpy = jest
Expand Down Expand Up @@ -222,45 +223,49 @@ describe('RemoteFeatureFlagController', () => {
state: { remoteFeatureFlags: [], cacheTimestamp: 0 },
});

let currentState;

// First call - should fetch new data
const firstFlags = await controller.getRemoteFeatureFlags();
expect(firstFlags).toStrictEqual(MOCK_FLAGS_WITH_NAMES);
await controller.updateRemoteFeatureFlags();
currentState = controller.state;
expect(currentState.remoteFeatureFlags).toStrictEqual(
MOCK_FLAGS_WITH_NAMES,
);
expect(fetchSpy).toHaveBeenCalledTimes(1);

// Advance time past cache duration
jest.setSystemTime(initialTime + 2 * DEFAULT_CACHE_DURATION);

// Second call - should fetch new data again
const secondFlags = await controller.getRemoteFeatureFlags();
expect(secondFlags).toStrictEqual(MOCK_FLAGS_TWO_WITH_NAMES);
await controller.updateRemoteFeatureFlags();
currentState = controller.state;
expect(currentState.remoteFeatureFlags).toStrictEqual(
MOCK_FLAGS_TWO_WITH_NAMES,
);
expect(fetchSpy).toHaveBeenCalledTimes(2);
});

it('should resolve with empty array when API returns no cached data', async () => {
// TODO: remove the handling of the empty array case
it('should handle empty data from API', async () => {
const clientConfigApiService = buildClientConfigApiService({
remoteFeatureFlags: [],
});

const controller = createController({ clientConfigApiService });
const result = await controller.getRemoteFeatureFlags();
const controller = createController({
clientConfigApiService,
state: {
remoteFeatureFlags: MOCK_FLAGS,
},
});
await controller.updateRemoteFeatureFlags();

expect(result).toStrictEqual([]);
expect(
clientConfigApiService.fetchRemoteFeatureFlags,
).toHaveBeenCalledTimes(1);
expect(controller.state.remoteFeatureFlags).toStrictEqual(MOCK_FLAGS);
});

it('should handle empty data from API', async () => {
const clientConfigApiService = buildClientConfigApiService({
remoteFeatureFlags: [],
});
const controller = createController({ clientConfigApiService });

const result = await controller.getRemoteFeatureFlags();
expect(result).toStrictEqual([]);
});

it('should handle errors during concurrent requests', async () => {
it('should throw an API error to the caller, while leaving cached state unchanged', async () => {
const clientConfigApiService = buildClientConfigApiService({
error: new Error('API Error'),
});
Expand All @@ -272,28 +277,10 @@ describe('RemoteFeatureFlagController', () => {
},
});

const result = await Promise.all([
controller.getRemoteFeatureFlags(),
controller.getRemoteFeatureFlags(),
]);
expect(result[0]).toStrictEqual(MOCK_FLAGS);
});

it('should preserve cache when API request fails', async () => {
const clientConfigApiService = buildClientConfigApiService();
const controller = createController({
clientConfigApiService,
state: { remoteFeatureFlags: MOCK_FLAGS, cacheTimestamp: 0 },
});

// Mock API to fail
jest
.spyOn(clientConfigApiService, 'fetchRemoteFeatureFlags')
.mockRejectedValue('API Error');

// Should still return cached data
const result = await controller.getRemoteFeatureFlags();
expect(result).toStrictEqual(MOCK_FLAGS);
await expect(
async () => await controller.updateRemoteFeatureFlags(),
).rejects.toThrow('API Error');
expect(controller.state.remoteFeatureFlags).toStrictEqual(MOCK_FLAGS);
});
});

Expand All @@ -306,7 +293,7 @@ describe('RemoteFeatureFlagController', () => {
});

controller.enable();
await controller.getRemoteFeatureFlags();
await controller.updateRemoteFeatureFlags();
expect(clientConfigApiService.fetchRemoteFeatureFlags).toHaveBeenCalled();
});

Expand All @@ -325,9 +312,9 @@ describe('RemoteFeatureFlagController', () => {
controller.disable();

// Verify disabled behavior,
// cache should be preserved, but getRemoteFeatureFlags should return empty array
const remoteFeatureFlags = await controller.getRemoteFeatureFlags();
expect(remoteFeatureFlags).toStrictEqual(MOCK_FLAGS);
// cache should be preserved, but updateRemoteFeatureFlags should return empty array
await controller.updateRemoteFeatureFlags();
expect(controller.state.remoteFeatureFlags).toStrictEqual(MOCK_FLAGS);
expect(controller.state.remoteFeatureFlags).toStrictEqual(MOCK_FLAGS);
});
});
Expand Down
26 changes: 9 additions & 17 deletions packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ export type RemoteFeatureFlagControllerGetStateAction =
>;

export type RemoteFeatureFlagControllerGetRemoteFeatureFlagAction = {
type: `${typeof controllerName}:getRemoteFeatureFlags`;
handler: RemoteFeatureFlagController['getRemoteFeatureFlags'];
type: `${typeof controllerName}:updateRemoteFeatureFlags`;
handler: RemoteFeatureFlagController['updateRemoteFeatureFlags'];
};

export type RemoteFeatureFlagControllerActions =
Expand Down Expand Up @@ -151,29 +151,23 @@ export class RemoteFeatureFlagController extends BaseController<
*
* @returns A promise that resolves to the current set of feature flags.
*/
async getRemoteFeatureFlags(): Promise<FeatureFlags> {
async updateRemoteFeatureFlags(): Promise<void> {
if (this.#disabled || !this.#isCacheExpired()) {
return this.state.remoteFeatureFlags;
return;
}

let serverData;

try {
if (this.#inProgressFlagUpdate) {
serverData = await this.#inProgressFlagUpdate;
const featureFlagsWithNames = this.getFeatureFlagsWithNames(
serverData.remoteFeatureFlags,
);
this.updateCache(featureFlagsWithNames);
return featureFlagsWithNames;
}
if (this.#inProgressFlagUpdate) {
await this.#inProgressFlagUpdate;
return;
}

try {
this.#inProgressFlagUpdate =
this.#clientConfigApiService.fetchRemoteFeatureFlags();

serverData = await this.#inProgressFlagUpdate;
} catch {
// Ignore
} finally {
this.#inProgressFlagUpdate = undefined;
}
Expand All @@ -183,9 +177,7 @@ export class RemoteFeatureFlagController extends BaseController<
serverData.remoteFeatureFlags,
);
this.updateCache(featureFlagsWithNames);
return featureFlagsWithNames;
}
return this.state.remoteFeatureFlags ?? []; // Resolve with cached state if no data is returned
}

/**
Expand Down

0 comments on commit 0f09ed5

Please sign in to comment.