From 576b66799c41bfd2853d7edb822d8413a928854e Mon Sep 17 00:00:00 2001 From: Francisco Ramini Date: Thu, 6 Jan 2022 12:08:52 +0100 Subject: [PATCH] Store cleanup (#396) * remove initial component subscribe, update component slice execute/failure reducers to allow non-existent ids * add basic cleanup saga and reducer * move logic to determine which components to clean to the saga to avoid dispatching too much * update connect to make the existence of the component optional * remove unneeded tests * fix tests * move logic to selector * extends tests utils to be able to pass a initial state * add test for getComponentsToCleanup * add basic test for the componentSaga * update saga test * add changeset * set cleanup delay to 1h Co-authored-by: Edoardo Gallo --- .changeset/tall-emus-change.md | 5 ++ packages/core/src/redux/connect.ts | 9 +--- .../features/component/componentSaga.test.ts | 53 +++++++++++++++++++ .../redux/features/component/componentSaga.ts | 23 ++++++++ .../component/componentSelectors.test.ts | 38 +++++++++++++ .../features/component/componentSelectors.ts | 19 ++++++- .../features/component/componentSlice.test.ts | 10 ---- .../features/component/componentSlice.ts | 29 ++++++---- packages/core/src/redux/index.ts | 2 +- packages/core/src/redux/rootSaga.test.ts | 2 + packages/core/src/redux/rootSaga.ts | 9 ++++ packages/core/src/testUtils.ts | 5 +- 12 files changed, 173 insertions(+), 31 deletions(-) create mode 100644 .changeset/tall-emus-change.md create mode 100644 packages/core/src/redux/features/component/componentSaga.test.ts create mode 100644 packages/core/src/redux/features/component/componentSaga.ts create mode 100644 packages/core/src/redux/features/component/componentSelectors.test.ts diff --git a/.changeset/tall-emus-change.md b/.changeset/tall-emus-change.md new file mode 100644 index 000000000..a7be6e304 --- /dev/null +++ b/.changeset/tall-emus-change.md @@ -0,0 +1,5 @@ +--- +'@signalwire/core': patch +--- + +[internal] Add ability to cleanup unused components from the store diff --git a/packages/core/src/redux/connect.ts b/packages/core/src/redux/connect.ts index 504ce1d4d..ee06c0d57 100644 --- a/packages/core/src/redux/connect.ts +++ b/packages/core/src/redux/connect.ts @@ -1,6 +1,5 @@ import { ReduxComponent, SessionState, CustomSaga } from './interfaces' import { SDKStore } from './' -import { componentActions } from './features' import { getComponent } from './features/component/componentSelectors' import { getSession } from './features/session/sessionSelectors' import type { BaseComponent } from '../BaseComponent' @@ -54,10 +53,7 @@ export const connect = < const storeUnsubscribe = store.subscribe(() => { const state = store.getState() - const component = getComponent(state, instance.__uuid) - if (!component) { - return - } + const component = getComponent(state, instance.__uuid) || {} for (const reduxKey of componentKeys) { if (run === false) { return @@ -65,7 +61,7 @@ export const connect = < const cacheKey = `${instance.__uuid}.${reduxKey}` const current = cacheMap.get(cacheKey) - const updatedValue = component?.[reduxKey] + const updatedValue = component[reduxKey] if (updatedValue !== undefined && current !== updatedValue) { cacheMap.set(cacheKey, updatedValue) const fnName = componentListeners[reduxKey] @@ -103,7 +99,6 @@ export const connect = < } } }) - store.dispatch(componentActions.upsert({ id: instance.__uuid })) // Run all the custom sagas const taskList = customSagas?.map((saga) => { diff --git a/packages/core/src/redux/features/component/componentSaga.test.ts b/packages/core/src/redux/features/component/componentSaga.test.ts new file mode 100644 index 000000000..2fbd51557 --- /dev/null +++ b/packages/core/src/redux/features/component/componentSaga.test.ts @@ -0,0 +1,53 @@ +import { Store } from 'redux' +import { expectSaga } from 'redux-saga-test-plan' +import { configureJestStore } from '../../../testUtils' +import { componentCleanupSagaWorker } from './componentSaga' +import { rootReducer } from '../../rootReducer' + +describe('componentCleanupSaga', () => { + let store: Store + + beforeEach(() => { + store = configureJestStore({ + preloadedState: { + components: { + byId: { + '268b4cf8-a3c5-4003-8666-3b7a4f0a5af9': { + id: '268b4cf8-a3c5-4003-8666-3b7a4f0a5af9', + }, + 'faa63915-3a64-4c39-acbb-06dac0758f8a': { + id: 'faa63915-3a64-4c39-acbb-06dac0758f8a', + responses: {}, + }, + 'zfaa63915-3a64-4c39-acbb-06dac0758f8a': { + id: 'zfaa63915-3a64-4c39-acbb-06dac0758f8a', + errors: {}, + }, + }, + }, + }, + }) + }) + it('should cleanup unused components from the store', () => { + return expectSaga(componentCleanupSagaWorker) + .withReducer(rootReducer, store.getState()) + .hasFinalState({ + components: { + byId: { + '268b4cf8-a3c5-4003-8666-3b7a4f0a5af9': { + id: '268b4cf8-a3c5-4003-8666-3b7a4f0a5af9', + }, + }, + }, + session: { + protocol: '', + iceServers: [], + authStatus: 'unknown', + authError: undefined, + authCount: 0, + }, + executeQueue: { queue: [] }, + }) + .run() + }) +}) diff --git a/packages/core/src/redux/features/component/componentSaga.ts b/packages/core/src/redux/features/component/componentSaga.ts new file mode 100644 index 000000000..c7d1842c1 --- /dev/null +++ b/packages/core/src/redux/features/component/componentSaga.ts @@ -0,0 +1,23 @@ +import { SagaIterator } from '@redux-saga/core' +import { delay, fork, put, select } from '@redux-saga/core/effects' +import { componentActions } from '..' +import { getComponentsToCleanup } from './componentSelectors' + +export function* componentCleanupSagaWorker(): SagaIterator { + const toCleanup = yield select(getComponentsToCleanup) + + if (toCleanup.length) { + yield put( + componentActions.cleanup({ + ids: toCleanup, + }) + ) + } +} + +export function* componentCleanupSaga(): SagaIterator { + while (true) { + yield delay(3600_000) // 1 hour + yield fork(componentCleanupSagaWorker) + } +} diff --git a/packages/core/src/redux/features/component/componentSelectors.test.ts b/packages/core/src/redux/features/component/componentSelectors.test.ts new file mode 100644 index 000000000..eb5214c03 --- /dev/null +++ b/packages/core/src/redux/features/component/componentSelectors.test.ts @@ -0,0 +1,38 @@ +import { Store } from 'redux' +import { configureJestStore } from '../../../testUtils' +import { getComponentsToCleanup } from './componentSelectors' + +describe('componentSelectors', () => { + let store: Store + + beforeEach(() => { + store = configureJestStore({ + preloadedState: { + components: { + byId: { + '268b4cf8-a3c5-4003-8666-3b7a4f0a5af9': { + id: '268b4cf8-a3c5-4003-8666-3b7a4f0a5af9', + }, + 'faa63915-3a64-4c39-acbb-06dac0758f8a': { + id: 'faa63915-3a64-4c39-acbb-06dac0758f8a', + responses: {}, + }, + 'zfaa63915-3a64-4c39-acbb-06dac0758f8a': { + id: 'zfaa63915-3a64-4c39-acbb-06dac0758f8a', + errors: {}, + }, + }, + }, + }, + }) + }) + + describe('getComponentsToCleanup', () => { + it('should return the list of components to cleanup', () => { + expect(getComponentsToCleanup(store.getState())).toEqual([ + 'faa63915-3a64-4c39-acbb-06dac0758f8a', + 'zfaa63915-3a64-4c39-acbb-06dac0758f8a', + ]) + }) + }) +}) diff --git a/packages/core/src/redux/features/component/componentSelectors.ts b/packages/core/src/redux/features/component/componentSelectors.ts index a9e9dbe4a..b692c628c 100644 --- a/packages/core/src/redux/features/component/componentSelectors.ts +++ b/packages/core/src/redux/features/component/componentSelectors.ts @@ -1,5 +1,22 @@ -import { SDKState } from '../../interfaces' +import { ReduxComponent, SDKState } from '../../interfaces' export const getComponent = ({ components }: SDKState, id: string) => { return components.byId?.[id] } + +export const getComponentsById = ({ components }: SDKState) => { + return components.byId +} + +export const getComponentsToCleanup = (state: SDKState) => { + const components = getComponentsById(state) + + let toCleanup: Array = [] + Object.keys(components).forEach((id) => { + if (components[id].responses || components[id].errors) { + toCleanup.push(id) + } + }) + + return toCleanup +} diff --git a/packages/core/src/redux/features/component/componentSlice.test.ts b/packages/core/src/redux/features/component/componentSlice.test.ts index 7d645f215..62282b4bf 100644 --- a/packages/core/src/redux/features/component/componentSlice.test.ts +++ b/packages/core/src/redux/features/component/componentSlice.test.ts @@ -62,11 +62,6 @@ describe('ComponentState Tests', () => { response, }) - it('should not change the state if the componentId does not exist', () => { - store.dispatch(executeSuccessAction) - expect(store.getState().components).toStrictEqual(initialComponentState) - }) - it('should update the state properly including the response', () => { // Create the component first store.dispatch(componentActions.upsert(component)) @@ -103,11 +98,6 @@ describe('ComponentState Tests', () => { error, }) - it('should not change the state if the componentId does not exist', () => { - store.dispatch(executeFailureAction) - expect(store.getState().components).toStrictEqual(initialComponentState) - }) - it('should update the state properly including both the action request and the error response', () => { // Create the component first store.dispatch(componentActions.upsert(component)) diff --git a/packages/core/src/redux/features/component/componentSlice.ts b/packages/core/src/redux/features/component/componentSlice.ts index 770c26adf..60cdbe5d2 100644 --- a/packages/core/src/redux/features/component/componentSlice.ts +++ b/packages/core/src/redux/features/component/componentSlice.ts @@ -8,6 +8,9 @@ export const initialComponentState: Readonly = { } type UpdateComponent = Partial & Pick +type CleanupComponentParams = { + ids: Array +} type SuccessParams = { componentId: string @@ -39,22 +42,28 @@ const componentSlice = createDestroyableSlice({ }, executeSuccess: (state, { payload }: PayloadAction) => { const { componentId, requestId, response } = payload - if (state.byId[componentId]) { - state.byId[componentId].responses = - state.byId[componentId].responses || {} - state.byId[componentId].responses![requestId] = response + state.byId[componentId] ??= { + id: componentId, } + state.byId[componentId].responses ??= {} + state.byId[componentId].responses![requestId] = response }, executeFailure: (state, { payload }: PayloadAction) => { const { componentId, requestId, error, action } = payload - if (state.byId[componentId]) { - state.byId[componentId].errors = state.byId[componentId].errors || {} - state.byId[componentId].errors![requestId] = { - action, - jsonrpc: error, - } + state.byId[componentId] ??= { + id: componentId, + } + state.byId[componentId].errors ??= {} + state.byId[componentId].errors![requestId] = { + action, + jsonrpc: error, } }, + cleanup: (state, { payload }: PayloadAction) => { + payload.ids.forEach((componentId) => { + delete state.byId[componentId] + }) + }, }, }) diff --git a/packages/core/src/redux/index.ts b/packages/core/src/redux/index.ts index e790179ad..63828b3fd 100644 --- a/packages/core/src/redux/index.ts +++ b/packages/core/src/redux/index.ts @@ -10,7 +10,7 @@ import { InternalChannels, } from '../utils/interfaces' -interface ConfigureStoreOptions { +export interface ConfigureStoreOptions { userOptions: InternalUserOptions SessionConstructor: SessionConstructor runSagaMiddleware?: boolean diff --git a/packages/core/src/redux/rootSaga.test.ts b/packages/core/src/redux/rootSaga.test.ts index 3968e8e9c..b96ca4ba5 100644 --- a/packages/core/src/redux/rootSaga.test.ts +++ b/packages/core/src/redux/rootSaga.test.ts @@ -31,6 +31,7 @@ import { } from './actions' import { AuthError } from '../CustomErrors' import { createPubSubChannel } from '../testUtils' +import { componentCleanupSaga } from './features/component/componentSaga' describe('socketClosedWorker', () => { it('should try to reconnect when session status is reconnecting', async () => { @@ -169,6 +170,7 @@ describe('initSessionSaga', () => { pubSubChannel, userOptions, }) + saga.next().fork(componentCleanupSaga) saga.next().take(destroyAction.type) saga.next().isDone() expect(pubSubChannel.close).toHaveBeenCalledTimes(1) diff --git a/packages/core/src/redux/rootSaga.ts b/packages/core/src/redux/rootSaga.ts index e1c09139a..8d3b0304f 100644 --- a/packages/core/src/redux/rootSaga.ts +++ b/packages/core/src/redux/rootSaga.ts @@ -37,6 +37,7 @@ import { import { AuthError } from '../CustomErrors' import { PubSubChannel } from './interfaces' import { createRestartableSaga } from './utils/sagaHelpers' +import { componentCleanupSaga } from './features/component/componentSaga' interface StartSagaOptions { session: BaseSession @@ -98,9 +99,17 @@ export function* initSessionSaga({ userOptions, }) + const compCleanupTask = yield fork(componentCleanupSaga) + session.connect() yield take(destroyAction.type) + /** + * We have to manually cancel the fork because it is not + * being automatically cleaned up when the session is + * destroyed, most likely because it's using a timer. + */ + compCleanupTask?.cancel() pubSubChannel.close() sessionChannel.close() customTasks.forEach((task) => task.cancel()) diff --git a/packages/core/src/testUtils.ts b/packages/core/src/testUtils.ts index 79cd0ec50..046d86151 100644 --- a/packages/core/src/testUtils.ts +++ b/packages/core/src/testUtils.ts @@ -1,5 +1,5 @@ import { channel } from '@redux-saga/core' -import { configureStore } from './redux' +import { configureStore, ConfigureStoreOptions } from './redux' import { PubSubChannel } from './redux/interfaces' import { BaseSession } from './BaseSession' import { RPCConnectResult, InternalSDKLogger } from './utils/interfaces' @@ -24,7 +24,7 @@ export const createMockedLogger = (): InternalSDKLogger => ({ * * @returns Redux Store */ -export const configureJestStore = () => { +export const configureJestStore = (options?: Partial) => { return configureStore({ userOptions: { project: PROJECT_ID, @@ -34,6 +34,7 @@ export const configureJestStore = () => { }, SessionConstructor: BaseSession, runSagaMiddleware: false, + ...options, }) }