-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Endpoint] Basic Functionality Alert List #55800
Changes from all commits
e5a6278
ff349af
806dc49
3ec2901
d05a80b
d8d5475
919e304
1063eda
49312ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { EndpointListAction } from './endpoint_list'; | ||
import { AlertAction } from './alerts'; | ||
import { RoutingAction } from './routing'; | ||
|
||
export type AppAction = EndpointListAction | AlertAction | RoutingAction; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { AlertData, Immutable } from '../../../../../common/types'; | ||
|
||
type ServerReturnedAlertsData = Immutable<{ | ||
type: 'serverReturnedAlertsData'; | ||
payload: AlertData[]; | ||
}>; | ||
|
||
export type AlertAction = ServerReturnedAlertsData; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
/* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are using a simple middleware to handle side-effects here, as opposed to using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm good with this approach as well - much simpler There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @peluja1012 what are some of the main differences here? It looks like we're still using Let me know what I'm missing @oatkiller @paul-tavares @parkiino I'm all for simplicity, just wanna better understand what we're proposing that we remove. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we're proposing to remove anything. Just that the alerting feature will (initially anyway) forgo The logic consists of:
Redux middleware, by default, provides the ability to run code on each action. |
||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { AlertData, ImmutableArray } from '../../../../../common/types'; | ||
import { AppAction } from '../action'; | ||
import { MiddlewareFactory } from '../../types'; | ||
|
||
export const alertMiddlewareFactory: MiddlewareFactory = coreStart => { | ||
return api => next => async (action: AppAction) => { | ||
next(action); | ||
if (action.type === 'userNavigatedToPage' && action.payload === 'alertsPage') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm assuming by your above There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @alexk307 We define actions in files like this one https://github.com/elastic/kibana/pull/55800/files#diff-c971622c4d5a45ad624ef764513a0131. The type checker will throw and error if you use an invalid action type in this if-statement. So if we were to do |
||
const response: ImmutableArray<AlertData> = await coreStart.http.get('/api/endpoint/alerts'); | ||
api.dispatch({ type: 'serverReturnedAlertsData', payload: response }); | ||
} | ||
}; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { Reducer } from 'redux'; | ||
import { AlertListState } from '../../types'; | ||
import { AppAction } from '../action'; | ||
|
||
const initialState = (): AlertListState => { | ||
return { | ||
alerts: [], | ||
}; | ||
}; | ||
|
||
export const alertListReducer: Reducer<AlertListState, AppAction> = ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you using Some suggestions: The feature (aka concern) is called 'alerting' and the directory takes this name. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will address this in the next PR where I add a stub alert detail |
||
state = initialState(), | ||
action | ||
) => { | ||
if (action.type === 'serverReturnedAlertsData') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may want to consider (in the future) "reseting" the state when a user leaves the Alerts list so that data stored here does not take up memory in the browser. |
||
return { | ||
...state, | ||
alerts: action.payload, | ||
}; | ||
} | ||
|
||
return state; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { AlertListState } from '../../types'; | ||
|
||
export const alertListData = (state: AlertListState) => state.alerts; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider naming this type There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will address this in the next PR where I add a stub alert detail |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,8 +4,9 @@ | |
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { Reducer } from 'redux'; | ||
import { EndpointListState } from './types'; | ||
import { EndpointListAction } from './action'; | ||
import { AppAction } from '../action'; | ||
|
||
const initialState = (): EndpointListState => { | ||
return { | ||
|
@@ -16,7 +17,10 @@ const initialState = (): EndpointListState => { | |
}; | ||
}; | ||
|
||
export const endpointListReducer = (state = initialState(), action: EndpointListAction) => { | ||
export const endpointListReducer: Reducer<EndpointListState, AppAction> = ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q. Why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. its because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah Ok - but were you getting TS errors? I think the benefit was that types shown here would be narrowed down to only the ones this concern cares about. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We weren't getting errors but I think to your previous point it might be good to take advantage of being able to use other concern's or generic/global app actions in the future and this would keep that ability. Kind of like how we have the page navigated actions now. I do see what you're saying though, i don't necessarily disagree with it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. even if we filter out non-concern specific actions in the future, they aren't yet filtered out, so this type is more correct |
||
state = initialState(), | ||
action | ||
) => { | ||
if (action.type === 'serverReturnedEndpointList') { | ||
return { | ||
...state, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { PageId } from '../../../../../common/types'; | ||
|
||
interface UserNavigatedToPage { | ||
readonly type: 'userNavigatedToPage'; | ||
readonly payload: PageId; | ||
} | ||
|
||
export type RoutingAction = UserNavigatedToPage; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
export { RoutingAction } from './action'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { GlobalState } from '../types'; | ||
import * as alertListSelectors from './alerts/selectors'; | ||
|
||
export const alertListData = composeSelectors( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI. File: export function useEndpointListSelector<TSelected>(
selector: (state: EndpointListState) => TSelected
) {
return useSelector(function(state: GlobalState) {
return selector(state.endpointList);
});
} The idea is that each of the concerns will have an associated hook. To use this in a component, you would: import { endpointListData } from "../store/endpoint_list/selectors;
import { useEndpointListSelector } from "../store/hooks;
const EndpointsListPage = () => {
const listData = useEndpointListSelector(endpointListData);
return (<div>...</div>)
} (Credit to @oatkiller for the idea during a recent discussion 😃 ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @paul-tavares If we use this approach, how would a middleware (or saga) use the selector if needed to grab some data from state? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @peluja1012 - From a saga or middleware we have access directly to the When we discussed this, the goal was to avoid the need to define all concerns selectors a second time in order for us to continue to use |
||
alertListStateSelector, | ||
alertListSelectors.alertListData | ||
); | ||
|
||
/** | ||
* Returns the alert list state from within Global State | ||
*/ | ||
function alertListStateSelector(state: GlobalState) { | ||
return state.alertList; | ||
} | ||
|
||
/** | ||
* Calls the `secondSelector` with the result of the `selector`. Use this when re-exporting a | ||
* concern-specific selector. `selector` should return the concern-specific state. | ||
*/ | ||
function composeSelectors<OuterState, InnerState, ReturnValue>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
selector: (state: OuterState) => InnerState, | ||
secondSelector: (state: InnerState) => ReturnValue | ||
): (state: OuterState) => ReturnValue { | ||
return state => secondSelector(selector(state)); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { Dispatch, MiddlewareAPI } from 'redux'; | ||
import { CoreStart } from 'kibana/public'; | ||
import { Immutable, AlertData } from '../../../common/types'; | ||
import { EndpointListState } from './store/endpoint_list'; | ||
import { AppAction } from './store/action'; | ||
|
||
export type MiddlewareFactory = ( | ||
coreStart: CoreStart | ||
) => ( | ||
api: MiddlewareAPI<Dispatch<AppAction>, GlobalState> | ||
) => (next: Dispatch<AppAction>) => (action: AppAction) => unknown; | ||
|
||
export type AlertListState = Immutable<{ | ||
alerts: AlertData[]; | ||
}>; | ||
|
||
export interface GlobalState { | ||
readonly endpointList: EndpointListState; | ||
readonly alertList: AlertListState; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a comment to all exported identifiers