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

middleware receive immutable versions of state and actions #63802

Merged
merged 2 commits into from
Apr 17, 2020
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ import { RoutingAction } from './routing';
import { PolicyListAction } from './policy_list';
import { PolicyDetailsAction } from './policy_details';

/**
* The entire set of redux actions recognized by our reducer.
*/
export type AppAction =
| HostAction
| AlertAction
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,15 @@

import { IIndexPattern } from 'src/plugins/data/public';
import { AlertResultList, AlertDetails } from '../../../../../common/types';
import { AppAction } from '../action';
import { MiddlewareFactory, AlertListState } from '../../types';
import { ImmutableMiddlewareFactory, AlertListState } from '../../types';
import { isOnAlertPage, apiQueryParams, hasSelectedAlert, uiQueryParams } from './selectors';
import { cloneHttpFetchQuery } from '../../../../common/clone_http_fetch_query';
import { EndpointAppConstants } from '../../../../../common/types';

export const alertMiddlewareFactory: MiddlewareFactory<AlertListState> = (coreStart, depsStart) => {
export const alertMiddlewareFactory: ImmutableMiddlewareFactory<AlertListState> = (
coreStart,
depsStart
) => {
async function fetchIndexPatterns(): Promise<IIndexPattern[]> {
const { indexPatterns } = depsStart.data;
const eventsPattern: { indexPattern: string } = await coreStart.http.get(
Expand All @@ -29,7 +31,7 @@ export const alertMiddlewareFactory: MiddlewareFactory<AlertListState> = (coreSt
return [indexPattern];
}

return api => next => async (action: AppAction) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will now be an Immutable version of AppAction.

return api => next => async action => {
next(action);
const state = api.getState();
if (action.type === 'userChangedUrl' && isOnAlertPage(state)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,12 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { MiddlewareFactory } from '../../types';
import { ImmutableMiddlewareFactory } from '../../types';
import { pageIndex, pageSize, isOnHostPage, hasSelectedHost, uiQueryParams } from './selectors';
import { HostListState } from '../../types';
import { AppAction } from '../action';

export const hostMiddlewareFactory: MiddlewareFactory<HostListState> = coreStart => {
return ({ getState, dispatch }) => next => async (action: AppAction) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will now be an Immutable version of AppAction.

export const hostMiddlewareFactory: ImmutableMiddlewareFactory<HostListState> = coreStart => {
return ({ getState, dispatch }) => next => async action => {
next(action);
const state = getState();
if (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,44 +4,25 @@
* you may not use this file except in compliance with the Elastic License.
*/

import {
createStore,
compose,
applyMiddleware,
Store,
MiddlewareAPI,
Dispatch,
Middleware,
} from 'redux';
import { createStore, compose, applyMiddleware, Store } from 'redux';
import { CoreStart } from 'kibana/public';
import { appReducer } from './reducer';
import { alertMiddlewareFactory } from './alerts/middleware';
import { hostMiddlewareFactory } from './hosts';
import { policyListMiddlewareFactory } from './policy_list';
import { policyDetailsMiddlewareFactory } from './policy_details';
import { GlobalState, MiddlewareFactory } from '../types';
import { AppAction } from './action';
import { ImmutableMiddlewareFactory, SubstateMiddlewareFactory } from '../types';
import { EndpointPluginStartDependencies } from '../../../plugin';

const composeWithReduxDevTools = (window as any).__REDUX_DEVTOOLS_EXTENSION_COMPOSE__
? (window as any).__REDUX_DEVTOOLS_EXTENSION_COMPOSE__({ name: 'EndpointApp' })
: compose;

export type Selector<S, R> = (state: S) => R;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved type to types module. Code should be more readable now, and this type is relevant to the others there: ImmutableMiddleware, ImmutableMiddlewareAPI, ImmutableMidlewareFactory.


/**
* Wrap Redux Middleware and adjust 'getState()' to return the namespace from 'GlobalState that applies to the given Middleware concern.
*
* @param selector
* @param middleware
*/
export const substateMiddlewareFactory = <Substate>(
selector: Selector<GlobalState, Substate>,
middleware: Middleware<{}, Substate, Dispatch<AppAction>>
): Middleware<{}, GlobalState, Dispatch<AppAction>> => {
export const substateMiddlewareFactory: SubstateMiddlewareFactory = (selector, middleware) => {
return api => {
const substateAPI: MiddlewareAPI<Dispatch<AppAction>, Substate> = {
const substateAPI = {
...api,
// Return just the substate instead of global state.
getState() {
return selector(api.getState());
},
Expand All @@ -66,7 +47,7 @@ export const appStoreFactory: (middlewareDeps?: {
* Any additional Redux Middlewares
* (should only be used for testing - example: to inject the action spy middleware)
*/
additionalMiddleware?: Array<ReturnType<MiddlewareFactory>>;
additionalMiddleware?: Array<ReturnType<ImmutableMiddlewareFactory>>;
Copy link
Contributor Author

@oatkiller oatkiller Apr 17, 2020

Choose a reason for hiding this comment

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

@paul-tavares objections to replacing this with:

actionSpyMiddleware?: ActionSpyMiddleware

I don't think code outside of the store should dictating what middleware are used, how they are constructed, or what order they are applied in.

Theoretical example of what i'm concerned about:

Assume we learn that the actionSpyMiddleware should really come before all other middleware (seems reasonable, since by being added last, other middleware could decide not to pass an action on to the action spy.) If we move all 'additional middleware' to the beginning of the chain, and if there were other additional middleware (as opposed to just the spy) then that could cause issues.

}) => Store = middlewareDeps => {
let middleware;
if (middlewareDeps) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { MiddlewareFactory, PolicyDetailsState, UpdatePolicyResponse } from '../../types';
import { ImmutableMiddlewareFactory, PolicyDetailsState, UpdatePolicyResponse } from '../../types';
import { policyIdFromParams, isOnPolicyDetailsPage, policyDetails } from './selectors';
import {
sendGetDatasource,
Expand All @@ -14,7 +14,7 @@ import {
import { PolicyData } from '../../../../../common/types';
import { factory as policyConfigFactory } from '../../../../../common/models/policy_config';

export const policyDetailsMiddlewareFactory: MiddlewareFactory<PolicyDetailsState> = coreStart => {
export const policyDetailsMiddlewareFactory: ImmutableMiddlewareFactory<PolicyDetailsState> = coreStart => {
const http = coreStart.http;

return ({ getState, dispatch }) => next => async action => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ export const isOnPolicyDetailsPage = (state: Immutable<PolicyDetailsState>) => {
};

/** Returns the policyId from the url */
export const policyIdFromParams: (state: PolicyDetailsState) => string = createSelector(
(state: PolicyDetailsState) => state.location,
export const policyIdFromParams: (state: Immutable<PolicyDetailsState>) => string = createSelector(
state => state.location,
(location: PolicyDetailsState['location']) => {
if (location) {
return location.pathname.split('/')[2];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { GetPolicyListResponse, MiddlewareFactory, PolicyListState } from '../../types';
import { GetPolicyListResponse, ImmutableMiddlewareFactory, PolicyListState } from '../../types';
import { sendGetEndpointSpecificDatasources } from './services/ingest';
import { isOnPolicyListPage, urlSearchParams } from './selectors';

export const policyListMiddlewareFactory: MiddlewareFactory<PolicyListState> = coreStart => {
export const policyListMiddlewareFactory: ImmutableMiddlewareFactory<PolicyListState> = coreStart => {
const http = coreStart.http;

return ({ getState, dispatch }) => next => async action => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/

import { Dispatch } from 'redux';
import { AppAction, GlobalState, MiddlewareFactory } from '../types';
import { AppAction, GlobalState, ImmutableMiddlewareFactory } from '../types';

/**
* Utilities for testing Redux middleware
Expand Down Expand Up @@ -35,7 +35,7 @@ export interface MiddlewareActionSpyHelper<S = GlobalState, A extends AppAction
/**
* Redux middleware that enables spying on the action that are dispatched through the store
*/
actionSpyMiddleware: ReturnType<MiddlewareFactory<S>>;
actionSpyMiddleware: ReturnType<ImmutableMiddlewareFactory<S>>;
}

/**
Expand Down Expand Up @@ -109,7 +109,7 @@ export const createSpyMiddleware = <
return spyDispatch.mock;
},

actionSpyMiddleware: api => {
actionSpyMiddleware: () => {
return next => {
spyDispatch = jest.fn(action => {
next(action);
Expand Down
63 changes: 58 additions & 5 deletions x-pack/plugins/endpoint/public/applications/endpoint/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,13 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { Dispatch, MiddlewareAPI, Action as ReduxAction, AnyAction as ReduxAnyAction } from 'redux';
import {
Dispatch,
Action as ReduxAction,
AnyAction as ReduxAnyAction,
Action,
Middleware,
} from 'redux';
import { IIndexPattern } from 'src/plugins/data/public';
import {
HostMetadata,
Expand All @@ -28,12 +34,59 @@ import {
} from '../../../../ingest_manager/common';

export { AppAction };
export type MiddlewareFactory<S = GlobalState> = (

/**
* like redux's `MiddlewareAPI` but `getState` returns an `Immutable` version of
* state and `dispatch` accepts `Immutable` versions of actions.
*/
export interface ImmutableMiddlewareAPI<S, A extends Action> {
dispatch: Dispatch<A | Immutable<A>>;
getState(): Immutable<S>;
}

/**
* Like redux's `Middleware` but without the ability to mutate actions or state.
* Differences:
* * `getState` returns an `Immutable` version of state
* * `dispatch` accepts `Immutable` versions of actions
* * `action`s received will be `Immutable`
*/
export type ImmutableMiddleware<S, A extends Action> = (
api: ImmutableMiddlewareAPI<S, A>
) => (next: Dispatch<A | Immutable<A>>) => (action: Immutable<A>) => unknown;

/**
* Takes application-standard middleware dependencies
* and returns a redux middleware.
* Middleware will be of the `ImmutableMiddleware` variety. Not able to directly
* change actions or state.
*/
export type ImmutableMiddlewareFactory<S = GlobalState> = (
coreStart: CoreStart,
depsStart: EndpointPluginStartDependencies
) => (
api: MiddlewareAPI<Dispatch<AppAction>, S>
) => (next: Dispatch<AppAction>) => (action: AppAction) => unknown;
) => ImmutableMiddleware<S, AppAction>;

/**
* Simple type for a redux selector.
*/
type Selector<S, R> = (state: S) => R;

/**
* Takes a selector and an `ImmutableMiddleware`. The
* middleware's version of `getState` will receive
* the result of the selector instead of the global state.
*
* This allows middleware to have knowledge of only a subsection of state.
*
* `selector` returns an `Immutable` version of the substate.
* `middleware` must be an `ImmutableMiddleware`.
*
* Returns a regular middleware, meant to be used with `applyMiddleware`.
*/
export type SubstateMiddlewareFactory = <Substate>(
selector: Selector<GlobalState, Immutable<Substate>>,
middleware: ImmutableMiddleware<Substate, AppAction>
) => Middleware<{}, GlobalState, Dispatch<AppAction | Immutable<AppAction>>>;

export interface HostListState {
hosts: HostMetadata[];
Expand Down