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

[Security Solution] Refactor useSelector #75297

Merged
11 changes: 11 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -826,6 +826,17 @@ module.exports = {
{
// prevents UI code from importing server side code and then webpack including it when doing builds
patterns: ['**/server/*'],
paths: [
/*
prevents importing raw useSelector which is using different equality function than mapStateToProps,
so to make sure we keep the logic while moving to hooks let's use useShallowEqualSelector
*/
{
name: 'react-redux',
importNames: ['useSelector'],
message: 'Please use "useShallowEqualSelector" instead or create your own selector',
Copy link
Contributor

Choose a reason for hiding this comment

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

you'll still get the error when you create your own selector correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct

Copy link
Contributor

@oatkiller oatkiller Aug 25, 2020

Choose a reason for hiding this comment

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

I don't think we should add a rule here. Many teams work in this code base, for that reason I think we should all use the exact set of lint rules defined by Kibana core.

The Resolver code base, for example, has (AFAIK) no reason to use shallow comparison for selectors as it was designed with the expectation that strict comparison would be used.

},
],
},
],
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@
*/

import React, { useState, useCallback, useMemo } from 'react';
import { useDispatch } from 'react-redux';

import { useDispatch, useSelector } from 'react-redux';
import { useShallowEqualSelector } from '../../../common/hooks/use_selector';
import { APP_ID } from '../../../../common/constants';
import { SecurityPageName } from '../../../app/types';
import { useKibana } from '../../../common/lib/kibana';
import { getCaseDetailsUrl, getCreateCaseUrl } from '../../../common/components/link_to';
import { State } from '../../../common/store';
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that we don't have to import State everywhere now

import { setInsertTimeline } from '../../../timelines/store/timeline/actions';
import { timelineSelectors } from '../../../timelines/store/timeline';

Expand All @@ -34,7 +34,7 @@ export const useAllCasesModal = ({
}: UseAllCasesModalProps): UseAllCasesModalReturnedValues => {
const dispatch = useDispatch();
const { navigateToApp } = useKibana().services.application;
const timeline = useSelector((state: State) =>
const timeline = useShallowEqualSelector((state) =>
timelineSelectors.selectTimeline(state, timelineId)
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,16 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { isEqual } from 'lodash/fp';
import { useMemo } from 'react';
import { useSelector } from 'react-redux';

import { useDeepEqualSelector } from '../../hooks/use_selector';
import { makeMapStateToProps } from '../url_state/helpers';
import { getSearch } from './helpers';
import { SearchNavTab } from './types';

export const useGetUrlSearch = (tab: SearchNavTab) => {
const mapState = makeMapStateToProps();
const { urlState } = useSelector(mapState, isEqual);
const { urlState } = useDeepEqualSelector(mapState);
const urlSearch = useMemo(() => getSearch(tab, urlState), [tab, urlState]);
return urlSearch;
};
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@
*/

import { useCallback, useMemo } from 'react';
import { useDispatch, useSelector } from 'react-redux';
import { SCROLLING_DISABLED_CLASS_NAME } from '../../../../common/constants';
import { useDispatch } from 'react-redux';

import { SCROLLING_DISABLED_CLASS_NAME } from '../../../../common/constants';
import { useShallowEqualSelector } from '../../hooks/use_selector';
import { inputsSelectors } from '../../store';
import { inputsActions } from '../../store/actions';

Expand All @@ -29,8 +30,10 @@ export const resetScroll = () => {

export const useFullScreen = () => {
const dispatch = useDispatch();
const globalFullScreen = useSelector(inputsSelectors.globalFullScreenSelector) ?? false;
const timelineFullScreen = useSelector(inputsSelectors.timelineFullScreenSelector) ?? false;
const globalFullScreen =
useShallowEqualSelector(inputsSelectors.globalFullScreenSelector) ?? false;
const timelineFullScreen =
useShallowEqualSelector(inputsSelectors.timelineFullScreenSelector) ?? false;

const setGlobalFullScreen = useCallback(
(fullScreen: boolean) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,16 @@
*/

import { useCallback, useState, useEffect, useMemo } from 'react';
import { useDispatch, useSelector } from 'react-redux';
import { useDispatch } from 'react-redux';

import { useShallowEqualSelector } from '../../hooks/use_selector';
import { inputsSelectors } from '../../store';
import { inputsActions } from '../../store/actions';
import { SetQuery, DeleteQuery } from './types';

export const useGlobalTime = (clearAllQuery: boolean = true) => {
const dispatch = useDispatch();
const { from, to } = useSelector(inputsSelectors.globalTimeRangeSelector);
const { from, to } = useShallowEqualSelector(inputsSelectors.globalTimeRangeSelector);
const [isInitializing, setIsInitializing] = useState(true);

const setQuery = useCallback(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* 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.
*/

// eslint-disable-next-line no-restricted-imports
import { shallowEqual, useSelector } from 'react-redux';
import deepEqual from 'fast-deep-equal';
import { State } from '../store';

export type TypedUseSelectorHook = <TSelected, TState = State>(
selector: (state: TState) => TSelected,
equalityFn?: (left: TSelected, right: TSelected) => boolean
) => TSelected;

export const useShallowEqualSelector: TypedUseSelectorHook = (selector) =>
useSelector(selector, shallowEqual);

export const useDeepEqualSelector: TypedUseSelectorHook = (selector) =>
useSelector(selector, deepEqual);
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@
import deepEqual from 'fast-deep-equal';
import { noop } from 'lodash/fp';
import { useCallback, useEffect, useRef, useState } from 'react';
import { useSelector } from 'react-redux';

import { DEFAULT_INDEX_KEY } from '../../../../common/constants';
import { HostsEdges, PageInfoPaginated } from '../../../graphql/types';
import { inputsModel, State } from '../../../common/store';
import { createFilter } from '../../../common/containers/helpers';
import { useKibana } from '../../../common/lib/kibana';
import { useShallowEqualSelector } from '../../../common/hooks/use_selector';
import { hostsModel, hostsSelectors } from '../../store';
import { generateTablePaginationOptions } from '../../../common/components/paginated_table/helpers';
import {
Expand Down Expand Up @@ -59,7 +59,7 @@ export const useAllHost = ({
type,
}: UseAllHost): [boolean, HostsArgs] => {
const getHostsSelector = hostsSelectors.hostsSelector();
const { activePage, direction, limit, sortField } = useSelector((state: State) =>
const { activePage, direction, limit, sortField } = useShallowEqualSelector((state: State) =>
getHostsSelector(state, type)
);
const { data, notifications, uiSettings } = useKibana().services;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,24 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { useSelector } from 'react-redux';
import { useMemo } from 'react';

import { useKibana } from '../../../../common/lib/kibana';
import { useShallowEqualSelector } from '../../../../common/hooks/use_selector';
import { EndpointState } from '../types';
import {
MANAGEMENT_STORE_ENDPOINTS_NAMESPACE,
MANAGEMENT_STORE_GLOBAL_NAMESPACE,
} from '../../../common/constants';
import { State } from '../../../../common/store';
export function useEndpointSelector<TSelected>(selector: (state: EndpointState) => TSelected) {
return useSelector(function (state: State) {
return selector(

export const useEndpointSelector = <TSelected>(selector: (state: EndpointState) => TSelected) =>
useShallowEqualSelector((state) =>
selector(
state[MANAGEMENT_STORE_GLOBAL_NAMESPACE][
MANAGEMENT_STORE_ENDPOINTS_NAMESPACE
] as EndpointState
);
});
}
)
);

/**
* Returns an object that contains Ingest app and URL information
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,8 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { useSelector } from 'react-redux';
import { PolicyListState, PolicyDetailsState } from '../types';
import { State } from '../../../../common/store';
import { useShallowEqualSelector } from '../../../../common/hooks/use_selector';
import {
MANAGEMENT_STORE_GLOBAL_NAMESPACE,
MANAGEMENT_STORE_POLICY_DETAILS_NAMESPACE,
Expand All @@ -17,28 +16,26 @@ import {
* Narrows global state down to the PolicyListState before calling the provided Policy List Selector
* @param selector
*/
export function usePolicyListSelector<TSelected>(selector: (state: PolicyListState) => TSelected) {
return useSelector((state: State) => {
return selector(
export const usePolicyListSelector = <TSelected>(selector: (state: PolicyListState) => TSelected) =>
useShallowEqualSelector((state) =>
selector(
state[MANAGEMENT_STORE_GLOBAL_NAMESPACE][
MANAGEMENT_STORE_POLICY_LIST_NAMESPACE
] as PolicyListState
);
});
}
)
);

/**
* Narrows global state down to the PolicyDetailsState before calling the provided Policy Details Selector
* @param selector
*/
export function usePolicyDetailsSelector<TSelected>(
export const usePolicyDetailsSelector = <TSelected>(
selector: (state: PolicyDetailsState) => TSelected
) {
return useSelector((state: State) =>
) =>
useShallowEqualSelector((state) =>
selector(
state[MANAGEMENT_STORE_GLOBAL_NAMESPACE][
MANAGEMENT_STORE_POLICY_DETAILS_NAMESPACE
] as PolicyDetailsState
)
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,14 @@
import React, { useCallback, useMemo, useContext } from 'react';
import styled from 'styled-components';
import { EuiRange, EuiPanel, EuiIcon } from '@elastic/eui';
import { useSelector, useDispatch } from 'react-redux';
import { useDispatch } from 'react-redux';

import { SideEffectContext } from './side_effect_context';
import { Vector2 } from '../types';
import * as selectors from '../store/selectors';
import { useResolverTheme } from './assets';
import { ResolverAction } from '../store/actions';
import { useShallowEqualSelector } from '../../common/hooks/use_selector';

interface StyledGraphControls {
graphControlsBackground: string;
Expand Down Expand Up @@ -64,7 +66,7 @@ const GraphControlsComponent = React.memo(
className?: string;
}) => {
const dispatch: (action: ResolverAction) => unknown = useDispatch();
const scalingFactor = useSelector(selectors.scalingFactor);
const scalingFactor = useShallowEqualSelector(selectors.scalingFactor);
const { timestamp } = useContext(SideEffectContext);
const { colorMap } = useResolverTheme();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@
*/

import React, { memo, useMemo, useContext, useLayoutEffect, useState } from 'react';
import { useSelector } from 'react-redux';
import { EuiPanel } from '@elastic/eui';

import * as selectors from '../../store/selectors';
import { useResolverDispatch } from '../use_resolver_dispatch';
import * as event from '../../../../common/endpoint/models/event';
import { useShallowEqualSelector } from '../../../common/hooks/use_selector';
import { ResolverEvent, ResolverNodeStats } from '../../../../common/endpoint/types';
import { SideEffectContext } from '../side_effect_context';
import { ProcessEventList } from './process_event_list';
Expand Down Expand Up @@ -41,7 +42,7 @@ const PanelContent = memo(function PanelContent() {

const { pushToQueryParams, queryParams } = useResolverQueryParams();

const graphableProcesses = useSelector(selectors.graphableProcesses);
const graphableProcesses = useShallowEqualSelector(selectors.graphableProcesses);
const graphableProcessEntityIds = useMemo(() => {
return new Set(graphableProcesses.map(event.entityId));
}, [graphableProcesses]);
Expand All @@ -60,7 +61,7 @@ const PanelContent = memo(function PanelContent() {
// The "selected" node (and its corresponding event) in the tree control.
// It may need to be synchronized with the ID indicated as selected via the `idFromParams`
// memo above. When this is the case, it is handled by the layout effect below.
const selectedNode = useSelector(selectors.selectedNode);
const selectedNode = useShallowEqualSelector(selectors.selectedNode);
const uiSelectedEvent = useMemo(() => {
return graphableProcesses.find((evt) => event.entityId(evt) === selectedNode);
}, [graphableProcesses, selectedNode]);
Expand Down Expand Up @@ -97,7 +98,7 @@ const PanelContent = memo(function PanelContent() {
}
}, [dispatch, uiSelectedEvent, paramsSelectedEvent, lastUpdatedProcess, timestamp]);

const relatedEventStats = useSelector(selectors.relatedEventsStats);
const relatedEventStats = useShallowEqualSelector(selectors.relatedEventsStats);
const { crumbId, crumbEvent } = queryParams;
const relatedStatsForIdFromParams: ResolverNodeStats | undefined = idFromParams
? relatedEventStats(idFromParams)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
* you may not use this file except in compliance with the Elastic License.
*/
import React, { memo, useMemo, HTMLAttributes } from 'react';
import { useSelector } from 'react-redux';
import { i18n } from '@kbn/i18n';
import {
htmlIdGenerator,
Expand All @@ -17,8 +16,10 @@ import {
import styled from 'styled-components';
import { FormattedMessage } from 'react-intl';
import { EuiDescriptionListProps } from '@elastic/eui/src/components/description_list/description_list';

import * as selectors from '../../store/selectors';
import * as event from '../../../../common/endpoint/models/event';
import { useShallowEqualSelector } from '../../../common/hooks/use_selector';
import { CrumbInfo, formatDate, StyledBreadcrumbs } from './panel_content_utilities';
import {
processPath,
Expand Down Expand Up @@ -51,7 +52,7 @@ export const ProcessDetails = memo(function ProcessDetails({
}) {
const processName = event.eventName(processEvent);
const entityId = event.entityId(processEvent);
const isProcessTerminated = useSelector(selectors.isProcessTerminated)(entityId);
const isProcessTerminated = useShallowEqualSelector(selectors.isProcessTerminated)(entityId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to this PR but: isProcessTerminated actually should be a parametric selector in order to work as expected.

const processInfoEntry: EuiDescriptionListProps['listItems'] = useMemo(() => {
const eventTime = event.eventTimestamp(processEvent);
const dateTime = eventTime === undefined ? null : formatDate(eventTime);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@
import React, { memo, useMemo, useEffect, Fragment } from 'react';
import { i18n } from '@kbn/i18n';
import { EuiTitle, EuiSpacer, EuiText, EuiButtonEmpty, EuiHorizontalRule } from '@elastic/eui';
import { useSelector } from 'react-redux';
import { FormattedMessage } from 'react-intl';
import styled from 'styled-components';

import {
CrumbInfo,
formatDate,
Expand All @@ -18,6 +18,7 @@ import {
StyledTime,
} from './panel_content_utilities';
import * as event from '../../../../common/endpoint/models/event';
import { useShallowEqualSelector } from '../../../common/hooks/use_selector';
import { ResolverEvent, ResolverNodeStats } from '../../../../common/endpoint/types';
import * as selectors from '../../store/selectors';
import { useResolverDispatch } from '../use_resolver_dispatch';
Expand Down Expand Up @@ -72,7 +73,7 @@ const DisplayList = memo(function DisplayList({
eventType: string;
processEntityId: string;
}) {
const relatedLookupsByCategory = useSelector(selectors.relatedEventInfoByEntityId);
const relatedLookupsByCategory = useShallowEqualSelector(selectors.relatedEventInfoByEntityId);
const lookupsForThisNode = relatedLookupsByCategory(processEntityId);
const shouldShowLimitWarning = lookupsForThisNode?.shouldShowLimitForCategory(eventType);
const numberDisplayed = lookupsForThisNode?.numberActuallyDisplayedForCategory(eventType);
Expand Down Expand Up @@ -160,7 +161,7 @@ export const ProcessEventList = memo(function ProcessEventList({
}
);

const relatedsReadyMap = useSelector(selectors.relatedEventsReady);
const relatedsReadyMap = useShallowEqualSelector(selectors.relatedEventsReady);
const relatedsReady = relatedsReadyMap.get(processEntityId);

const dispatch = useResolverDispatch();
Expand All @@ -185,7 +186,7 @@ export const ProcessEventList = memo(function ProcessEventList({
];
}, [pushToQueryParams, eventsString]);

const relatedByCategory = useSelector(selectors.relatedEventsByCategory);
const relatedByCategory = useShallowEqualSelector(selectors.relatedEventsByCategory);

/**
* A list entry will be displayed for each of these
Expand Down
Loading