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 <[email protected]>
signalwire · Jan 6, 2022 · 576b667 · 576b667
1 parent 2c8cc9b
commit 576b667
Show file tree

Hide file tree

Showing 12 changed files with 173 additions and 31 deletions.
diff --git a/.changeset/tall-emus-change.md 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
@@ -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,18 +53,15 @@ 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
         }
 
         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
@@ -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
@@ -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
@@ -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
@@ -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<ReduxComponent['id']> = []
+  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
@@ -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
@@ -8,6 +8,9 @@ export const initialComponentState: Readonly<ComponentState> = {
 }
 
 type UpdateComponent = Partial<ReduxComponent> & Pick<ReduxComponent, 'id'>
+type CleanupComponentParams = {
+  ids: Array<ReduxComponent['id']>
+}
 
 type SuccessParams = {
   componentId: string
@@ -39,22 +42,28 @@ const componentSlice = createDestroyableSlice({
     },
     executeSuccess: (state, { payload }: PayloadAction<SuccessParams>) => {
       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<FailureParams>) => {
       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<CleanupComponentParams>) => {
+      payload.ids.forEach((componentId) => {
+        delete state.byId[componentId]
+      })
+    },
   },
 })
 

diff --git 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
@@ -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
@@ -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
@@ -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<ConfigureStoreOptions>) => {
   return configureStore({
     userOptions: {
       project: PROJECT_ID,
@@ -34,6 +34,7 @@ export const configureJestStore = () => {
     },
     SessionConstructor: BaseSession,
     runSagaMiddleware: false,
+    ...options,
   })
 }