From 5a5bade8be51c2d93a3535dc9952e7ca47309dc0 Mon Sep 17 00:00:00 2001 From: Paul Tavares <56442535+paul-tavares@users.noreply.github.com> Date: Tue, 21 Jan 2020 14:21:33 -0500 Subject: [PATCH] [Endpoint] Fix saga to start only after store is created and stopped on app unmount (#55245) - added `stop()`/`start()` methods to the Saga Middleware creator factory - adjust tests based on changes - changed application `renderApp` to stop sagas when react app is unmounted --- .../public/applications/endpoint/index.tsx | 3 +- .../applications/endpoint/lib/saga.test.ts | 57 ++++++++++++------- .../public/applications/endpoint/lib/saga.ts | 40 +++++++++++-- .../endpoint/store/endpoint_list/saga.test.ts | 15 +++-- .../applications/endpoint/store/index.ts | 11 ++-- 5 files changed, 90 insertions(+), 36 deletions(-) diff --git a/x-pack/plugins/endpoint/public/applications/endpoint/index.tsx b/x-pack/plugins/endpoint/public/applications/endpoint/index.tsx index d69e068bdea3a..7598141bdea65 100644 --- a/x-pack/plugins/endpoint/public/applications/endpoint/index.tsx +++ b/x-pack/plugins/endpoint/public/applications/endpoint/index.tsx @@ -19,12 +19,13 @@ import { appStoreFactory } from './store'; export function renderApp(coreStart: CoreStart, { appBasePath, element }: AppMountParameters) { coreStart.http.get('/api/endpoint/hello-world'); - const store = appStoreFactory(coreStart); + const [store, stopSagas] = appStoreFactory(coreStart); ReactDOM.render(, element); return () => { ReactDOM.unmountComponentAtNode(element); + stopSagas(); }; } diff --git a/x-pack/plugins/endpoint/public/applications/endpoint/lib/saga.test.ts b/x-pack/plugins/endpoint/public/applications/endpoint/lib/saga.test.ts index 0387eac0e7c7f..91841f75c24fe 100644 --- a/x-pack/plugins/endpoint/public/applications/endpoint/lib/saga.test.ts +++ b/x-pack/plugins/endpoint/public/applications/endpoint/lib/saga.test.ts @@ -3,18 +3,21 @@ * or more contributor license agreements. Licensed under the Elastic License; * you may not use this file except in compliance with the Elastic License. */ -import { createSagaMiddleware, SagaContext } from './index'; -import { applyMiddleware, createStore, Reducer } from 'redux'; + +import { createSagaMiddleware, SagaContext, SagaMiddleware } from './index'; +import { applyMiddleware, createStore, Reducer, Store } from 'redux'; describe('saga', () => { const INCREMENT_COUNTER = 'INCREMENT'; const DELAYED_INCREMENT_COUNTER = 'DELAYED INCREMENT COUNTER'; const STOP_SAGA_PROCESSING = 'BREAK ASYNC ITERATOR'; - const sleep = (ms = 1000) => new Promise(resolve => setTimeout(resolve, ms)); + const sleep = (ms = 10) => new Promise(resolve => setTimeout(resolve, ms)); + let store: Store; let reducerA: Reducer; let sideAffect: (a: unknown, s: unknown) => void; let sagaExe: (sagaContext: SagaContext) => Promise; + let sagaExeReduxMiddleware: SagaMiddleware; beforeEach(() => { reducerA = jest.fn((prevState = { count: 0 }, { type }) => { @@ -47,53 +50,63 @@ describe('saga', () => { } } }); + + sagaExeReduxMiddleware = createSagaMiddleware(sagaExe); + store = createStore(reducerA, applyMiddleware(sagaExeReduxMiddleware)); }); - test('it returns Redux Middleware from createSagaMiddleware()', () => { - const sagaMiddleware = createSagaMiddleware(async () => {}); - expect(sagaMiddleware).toBeInstanceOf(Function); + afterEach(() => { + sagaExeReduxMiddleware.stop(); }); + test('it does nothing if saga is not started', () => { - const store = createStore(reducerA, applyMiddleware(createSagaMiddleware(sagaExe))); - expect(store.getState().count).toEqual(0); - expect(reducerA).toHaveBeenCalled(); - expect(sagaExe).toHaveBeenCalled(); - expect(sideAffect).not.toHaveBeenCalled(); - expect(store.getState()).toEqual({ count: 0 }); + expect(sagaExe).not.toHaveBeenCalled(); }); - test('it updates store once running', async () => { - const sagaMiddleware = createSagaMiddleware(sagaExe); - const store = createStore(reducerA, applyMiddleware(sagaMiddleware)); + test('it can dispatch store actions once running', async () => { + sagaExeReduxMiddleware.start(); expect(store.getState()).toEqual({ count: 0 }); expect(sagaExe).toHaveBeenCalled(); store.dispatch({ type: DELAYED_INCREMENT_COUNTER }); expect(store.getState()).toEqual({ count: 0 }); - await sleep(100); + await sleep(); expect(sideAffect).toHaveBeenCalled(); expect(store.getState()).toEqual({ count: 1 }); }); - test('it stops processing if break out of loop', async () => { - const sagaMiddleware = createSagaMiddleware(sagaExe); - const store = createStore(reducerA, applyMiddleware(sagaMiddleware)); + test('it stops processing if break out of loop', async () => { + sagaExeReduxMiddleware.start(); store.dispatch({ type: DELAYED_INCREMENT_COUNTER }); - await sleep(100); + await sleep(); expect(store.getState()).toEqual({ count: 1 }); expect(sideAffect).toHaveBeenCalledTimes(2); store.dispatch({ type: STOP_SAGA_PROCESSING }); - await sleep(100); + await sleep(); + + store.dispatch({ type: DELAYED_INCREMENT_COUNTER }); + await sleep(); + + expect(store.getState()).toEqual({ count: 1 }); + expect(sideAffect).toHaveBeenCalledTimes(2); + }); + + test('it stops saga middleware when stop() is called', async () => { + sagaExeReduxMiddleware.start(); + store.dispatch({ type: DELAYED_INCREMENT_COUNTER }); + await sleep(); expect(store.getState()).toEqual({ count: 1 }); expect(sideAffect).toHaveBeenCalledTimes(2); + sagaExeReduxMiddleware.stop(); + store.dispatch({ type: DELAYED_INCREMENT_COUNTER }); - await sleep(100); + await sleep(); expect(store.getState()).toEqual({ count: 1 }); expect(sideAffect).toHaveBeenCalledTimes(2); diff --git a/x-pack/plugins/endpoint/public/applications/endpoint/lib/saga.ts b/x-pack/plugins/endpoint/public/applications/endpoint/lib/saga.ts index b93360ec6b5aa..bca6aa6563fe5 100644 --- a/x-pack/plugins/endpoint/public/applications/endpoint/lib/saga.ts +++ b/x-pack/plugins/endpoint/public/applications/endpoint/lib/saga.ts @@ -35,7 +35,20 @@ export interface SagaContext { dispatch: Dispatch; } +export interface SagaMiddleware extends Middleware { + /** + * Start the saga. Should be called after the `store` has been created + */ + start: () => void; + + /** + * Stop the saga by exiting the internal generator `for await...of` loop. + */ + stop: () => void; +} + const noop = () => {}; +const STOP = Symbol('STOP'); /** * Creates Saga Middleware for use with Redux. @@ -43,7 +56,7 @@ const noop = () => {}; * @param {Saga} saga The `saga` should initialize a long-running `for await...of` loop against * the return value of the `actionsAndState()` method provided by the `SagaContext`. * - * @return {Middleware} + * @return {SagaMiddleware} * * @example * @@ -64,22 +77,31 @@ const noop = () => {}; * //.... * const store = createStore(reducers, [ endpointsSagaMiddleware ]); */ -export function createSagaMiddleware(saga: Saga): Middleware { +export function createSagaMiddleware(saga: Saga): SagaMiddleware { const iteratorInstances = new Set(); let runSaga: () => void = noop; + let stopSaga: () => void = noop; + let runningPromise: Promise; async function* getActionsAndStateIterator(): StoreActionsAndState { const instance: IteratorInstance = { queue: [], nextResolve: null }; iteratorInstances.add(instance); + try { while (true) { - yield await nextActionAndState(); + const actionAndState = await Promise.race([nextActionAndState(), runningPromise]); + + if (actionAndState === STOP) { + break; + } + + yield actionAndState as QueuedAction; } } finally { // If the consumer stops consuming this (e.g. `break` or `return` is called in the `for await` // then this `finally` block will run and unregister this instance and reset `runSaga` iteratorInstances.delete(instance); - runSaga = noop; + runSaga = stopSaga = noop; } function nextActionAndState() { @@ -109,7 +131,6 @@ export function createSagaMiddleware(saga: Saga): Middleware { actionsAndState: getActionsAndStateIterator, dispatch, }); - runSaga(); } return (next: Dispatch) => (action: AnyAction) => { // Call the next dispatch method in the middleware chain. @@ -125,5 +146,14 @@ export function createSagaMiddleware(saga: Saga): Middleware { }; } + middleware.start = () => { + runningPromise = new Promise(resolve => (stopSaga = () => resolve(STOP))); + runSaga(); + }; + + middleware.stop = () => { + stopSaga(); + }; + return middleware; } diff --git a/x-pack/plugins/endpoint/public/applications/endpoint/store/endpoint_list/saga.test.ts b/x-pack/plugins/endpoint/public/applications/endpoint/store/endpoint_list/saga.test.ts index 92bf3b7fd92dd..6bf946873e179 100644 --- a/x-pack/plugins/endpoint/public/applications/endpoint/store/endpoint_list/saga.test.ts +++ b/x-pack/plugins/endpoint/public/applications/endpoint/store/endpoint_list/saga.test.ts @@ -24,6 +24,7 @@ describe('endpoint list saga', () => { let fakeHttpServices: jest.Mocked; let store: Store; let dispatch: Dispatch; + let stopSagas: () => void; // TODO: consolidate the below ++ helpers in `index.test.ts` into a `test_helpers.ts`?? const generateEndpoint = (): EndpointData => { @@ -89,13 +90,19 @@ describe('endpoint list saga', () => { beforeEach(() => { fakeCoreStart = coreMock.createStart({ basePath: '/mock' }); fakeHttpServices = fakeCoreStart.http as jest.Mocked; - store = createStore( - endpointListReducer, - applyMiddleware(createSagaMiddleware(endpointListSagaFactory())) - ); + + const sagaMiddleware = createSagaMiddleware(endpointListSagaFactory()); + store = createStore(endpointListReducer, applyMiddleware(sagaMiddleware)); + + sagaMiddleware.start(); + stopSagas = sagaMiddleware.stop; dispatch = store.dispatch; }); + afterEach(() => { + stopSagas(); + }); + test('it handles `userEnteredEndpointListPage`', async () => { const apiResponse = getEndpointListApiResponse(); diff --git a/x-pack/plugins/endpoint/public/applications/endpoint/store/index.ts b/x-pack/plugins/endpoint/public/applications/endpoint/store/index.ts index d0dc002031ce2..bfa1385b9f0ac 100644 --- a/x-pack/plugins/endpoint/public/applications/endpoint/store/index.ts +++ b/x-pack/plugins/endpoint/public/applications/endpoint/store/index.ts @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import { createStore, compose, applyMiddleware } from 'redux'; +import { createStore, compose, applyMiddleware, Store } from 'redux'; import { CoreStart } from 'kibana/public'; import { appSagaFactory } from './saga'; import { appReducer } from './reducer'; @@ -15,10 +15,13 @@ const composeWithReduxDevTools = (window as any).__REDUX_DEVTOOLS_EXTENSION_COMP ? (window as any).__REDUX_DEVTOOLS_EXTENSION_COMPOSE__({ name: 'EndpointApp' }) : compose; -export const appStoreFactory = (coreStart: CoreStart) => { +export const appStoreFactory = (coreStart: CoreStart): [Store, () => void] => { + const sagaReduxMiddleware = appSagaFactory(coreStart); const store = createStore( appReducer, - composeWithReduxDevTools(applyMiddleware(appSagaFactory(coreStart))) + composeWithReduxDevTools(applyMiddleware(sagaReduxMiddleware)) ); - return store; + + sagaReduxMiddleware.start(); + return [store, sagaReduxMiddleware.stop]; };