From 214c285416c88c88e5aac66faae5d2262dafd494 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Mon, 18 Nov 2019 22:50:19 +0100 Subject: [PATCH 01/20] Remove infra eslint exception --- .eslintrc.js | 7 ------- 1 file changed, 7 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index 58b74d0bf5c25..68dd829c9d34f 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -176,13 +176,6 @@ module.exports = { 'react-hooks/rules-of-hooks': 'off', }, }, - { - files: ['x-pack/legacy/plugins/infra/**/*.{js,ts,tsx}'], - rules: { - 'react-hooks/exhaustive-deps': 'off', - 'react-hooks/rules-of-hooks': 'off', - }, - }, { files: ['x-pack/legacy/plugins/lens/**/*.{js,ts,tsx}'], rules: { From 9aa42ed46e4a4bb19b20cc7c1fbc3eaa572e2b19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Tue, 3 Dec 2019 00:34:53 +0100 Subject: [PATCH 02/20] Enhance useUrlState to write default to URL Instead of requiring separate `useEffect` usages to populate the URL with the initial state, enhance `useUrlState` with the ability to write back the default state to the URL if the `writeDefaultState` option is true. --- .../log_analysis_results_url_state.tsx | 16 +++------ .../infra/public/utils/use_url_state.ts | 34 +++++++++++++------ 2 files changed, 29 insertions(+), 21 deletions(-) diff --git a/x-pack/legacy/plugins/infra/public/containers/logs/log_analysis/log_analysis_results_url_state.tsx b/x-pack/legacy/plugins/infra/public/containers/logs/log_analysis/log_analysis_results_url_state.tsx index 19fb7f238fc04..6d4495c8d9e0f 100644 --- a/x-pack/legacy/plugins/infra/public/containers/logs/log_analysis/log_analysis_results_url_state.tsx +++ b/x-pack/legacy/plugins/infra/public/containers/logs/log_analysis/log_analysis_results_url_state.tsx @@ -4,11 +4,11 @@ * you may not use this file except in compliance with the Elastic License. */ -import { useEffect } from 'react'; -import * as rt from 'io-ts'; -import { identity, constant } from 'fp-ts/lib/function'; import { fold } from 'fp-ts/lib/Either'; +import { constant, identity } from 'fp-ts/lib/function'; import { pipe } from 'fp-ts/lib/pipeable'; +import * as rt from 'io-ts'; + import { useUrlState } from '../../../utils/use_url_state'; const autoRefreshRT = rt.union([ @@ -40,12 +40,9 @@ export const useLogAnalysisResultsUrlState = () => { pipe(urlTimeRangeRT.decode(value), fold(constant(undefined), identity)), encodeUrlState: urlTimeRangeRT.encode, urlStateKey: TIME_RANGE_URL_STATE_KEY, + writeDefaultState: true, }); - useEffect(() => { - setTimeRange(timeRange); - }, []); - const [autoRefresh, setAutoRefresh] = useUrlState({ defaultState: { isPaused: false, @@ -55,12 +52,9 @@ export const useLogAnalysisResultsUrlState = () => { pipe(autoRefreshRT.decode(value), fold(constant(undefined), identity)), encodeUrlState: autoRefreshRT.encode, urlStateKey: AUTOREFRESH_URL_STATE_KEY, + writeDefaultState: true, }); - useEffect(() => { - setAutoRefresh(autoRefresh); - }, []); - return { timeRange, setTimeRange, diff --git a/x-pack/legacy/plugins/infra/public/utils/use_url_state.ts b/x-pack/legacy/plugins/infra/public/utils/use_url_state.ts index d03a5aaa9d697..5f107c99ae86e 100644 --- a/x-pack/legacy/plugins/infra/public/utils/use_url_state.ts +++ b/x-pack/legacy/plugins/infra/public/utils/use_url_state.ts @@ -5,7 +5,7 @@ */ import { Location } from 'history'; -import { useMemo, useCallback } from 'react'; +import { useMemo, useCallback, useState } from 'react'; import { decode, encode, RisonValue } from 'rison-node'; import { QueryString } from 'ui/utils/query_string'; @@ -16,27 +16,36 @@ export const useUrlState = ({ decodeUrlState, encodeUrlState, urlStateKey, + writeDefaultState = false, }: { defaultState: State; decodeUrlState: (value: RisonValue | undefined) => State | undefined; encodeUrlState: (value: State) => RisonValue | undefined; urlStateKey: string; + writeDefaultState?: boolean; }) => { const history = useHistory(); + // history.location is mutable so we can't reliably use useMemo + const queryString = history?.location ? getQueryStringFromLocation(history.location) : ''; + const urlStateString = useMemo(() => { - if (!history) { + if (!queryString) { return; } - return getParamFromQueryString(getQueryStringFromLocation(history.location), urlStateKey); - }, [history && history.location, urlStateKey]); + return getParamFromQueryString(queryString, urlStateKey); + }, [queryString, urlStateKey]); const decodedState = useMemo(() => decodeUrlState(decodeRisonUrlState(urlStateString)), [ decodeUrlState, urlStateString, ]); + const [shouldInitialize, setShouldInitialize] = useState( + writeDefaultState && typeof decodedState === 'undefined' + ); + const state = useMemo(() => (typeof decodedState !== 'undefined' ? decodedState : defaultState), [ defaultState, decodedState, @@ -44,27 +53,32 @@ export const useUrlState = ({ const setState = useCallback( (newState: State | undefined) => { - if (!history) { + if (!history || !history.location) { return; } - const location = history.location; + const currentLocation = history.location; const newLocation = replaceQueryStringInLocation( - location, + currentLocation, replaceStateKeyInQueryString( urlStateKey, typeof newState !== 'undefined' ? encodeUrlState(newState) : undefined - )(getQueryStringFromLocation(location)) + )(getQueryStringFromLocation(currentLocation)) ); - if (newLocation !== location) { + if (newLocation !== currentLocation) { history.replace(newLocation); } }, - [encodeUrlState, history, history && history.location, urlStateKey] + [encodeUrlState, history, urlStateKey] ); + if (shouldInitialize) { + setShouldInitialize(false); + setState(defaultState); + } + return [state, setState] as [typeof state, typeof setState]; }; From 6a660d483bc1a4be94f659cc595e6a967217508a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Tue, 3 Dec 2019 00:44:09 +0100 Subject: [PATCH 03/20] Refactor autoFocus ref handler to avoid useEffect --- .../components/metrics_explorer/metrics.tsx | 31 ++++++++----------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/x-pack/legacy/plugins/infra/public/components/metrics_explorer/metrics.tsx b/x-pack/legacy/plugins/infra/public/components/metrics_explorer/metrics.tsx index d59e709d9a19a..5cc7878a0c26e 100644 --- a/x-pack/legacy/plugins/infra/public/components/metrics_explorer/metrics.tsx +++ b/x-pack/legacy/plugins/infra/public/components/metrics_explorer/metrics.tsx @@ -7,7 +7,7 @@ import { EuiComboBox } from '@elastic/eui'; import { i18n } from '@kbn/i18n'; -import React, { useCallback, useState, useEffect } from 'react'; +import React, { useCallback, useState } from 'react'; import { FieldType } from 'ui/index_patterns'; import { colorTransformer, MetricsExplorerColor } from '../../../common/color_palette'; import { @@ -31,24 +31,19 @@ interface SelectedOption { export const MetricsExplorerMetrics = ({ options, onChange, fields, autoFocus = false }: Props) => { const colors = Object.keys(MetricsExplorerColor) as MetricsExplorerColor[]; - const [inputRef, setInputRef] = useState(null); - const [focusOnce, setFocusState] = useState(false); + const [shouldFocus, setShouldFocus] = useState(autoFocus); - useEffect(() => { - if (inputRef && autoFocus && !focusOnce) { - inputRef.focus(); - setFocusState(true); - } - }, [inputRef]); + // the EuiCombobox forwards the ref to an input element + const handleInputRef = useCallback( + (ref: HTMLInputElement | null) => { + if (ref && shouldFocus) { + ref.focus(); + setShouldFocus(false); + } + }, + [shouldFocus] + ); - // I tried to use useRef originally but the EUIComboBox component's type definition - // would only accept an actual input element or a callback function (with the same type). - // This effectivly does the same thing but is compatible with EuiComboBox. - const handleInputRef = (ref: HTMLInputElement) => { - if (ref) { - setInputRef(ref); - } - }; const handleChange = useCallback( selectedOptions => { onChange( @@ -59,7 +54,7 @@ export const MetricsExplorerMetrics = ({ options, onChange, fields, autoFocus = })) ); }, - [options, onChange] + [onChange, options.aggregation, colors] ); const comboOptions = fields From 4e0f0db9414285f7e410e1033746a346ff4d6768 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Tue, 3 Dec 2019 00:46:01 +0100 Subject: [PATCH 04/20] Refactor onChange callback to avoid useEffect --- .../logging/log_highlights_menu.tsx | 24 +++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/x-pack/legacy/plugins/infra/public/components/logging/log_highlights_menu.tsx b/x-pack/legacy/plugins/infra/public/components/logging/log_highlights_menu.tsx index 24a5e8bacb4f9..d13ccde7466cd 100644 --- a/x-pack/legacy/plugins/infra/public/components/logging/log_highlights_menu.tsx +++ b/x-pack/legacy/plugins/infra/public/components/logging/log_highlights_menu.tsx @@ -16,7 +16,7 @@ import { import { i18n } from '@kbn/i18n'; import { FormattedMessage } from '@kbn/i18n/react'; import { debounce } from 'lodash'; -import React, { useCallback, useEffect, useMemo, useState } from 'react'; +import React, { useCallback, useMemo, useState } from 'react'; import euiStyled from '../../../../../common/eui_styled_components'; import { useVisibilityState } from '../../utils/use_visibility_state'; @@ -47,8 +47,25 @@ export const LogHighlightsMenu: React.FC = ({ } = useVisibilityState(false); // Input field state - const [highlightTerm, setHighlightTerm] = useState(''); + const [highlightTerm, _setHighlightTerm] = useState(''); + const debouncedOnChange = useMemo(() => debounce(onChange, 275), [onChange]); + const setHighlightTerm = useCallback( + valueOrUpdater => + _setHighlightTerm(previousHighlightTerm => { + const newHighlightTerm = + typeof valueOrUpdater === 'function' + ? valueOrUpdater(previousHighlightTerm) + : valueOrUpdater; + + if (newHighlightTerm !== previousHighlightTerm) { + debouncedOnChange([newHighlightTerm]); + } + + return newHighlightTerm; + }), + [debouncedOnChange] + ); const changeHighlightTerm = useCallback( e => { const value = e.target.value; @@ -57,9 +74,6 @@ export const LogHighlightsMenu: React.FC = ({ [setHighlightTerm] ); const clearHighlightTerm = useCallback(() => setHighlightTerm(''), [setHighlightTerm]); - useEffect(() => { - debouncedOnChange([highlightTerm]); - }, [highlightTerm]); const button = ( From c366a3638e22b8ae5de35a4c7c2b2588e4db801e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Tue, 3 Dec 2019 00:50:11 +0100 Subject: [PATCH 05/20] Fix incorrect dependency lists --- .../public/components/formatted_time.tsx | 1 - .../log_entry_actions_menu.tsx | 2 +- .../logging/log_text_stream/text_styles.tsx | 2 +- .../components/saved_views/create_modal.tsx | 2 +- .../add_log_column_popover.tsx | 2 +- .../source_configuration_form_state.tsx | 2 +- .../waffle/waffle_inventory_switcher.tsx | 27 +++++++++++-------- .../log_analysis_capabilities.tsx | 2 +- .../logs/log_analysis/log_analysis_jobs.tsx | 8 +++--- .../log_analysis/log_analysis_results.tsx | 2 +- .../log_highlights/log_entry_highlights.tsx | 2 +- .../log_highlights/log_summary_highlights.ts | 10 ++++++- 12 files changed, 37 insertions(+), 25 deletions(-) diff --git a/x-pack/legacy/plugins/infra/public/components/formatted_time.tsx b/x-pack/legacy/plugins/infra/public/components/formatted_time.tsx index 78255c55df124..46b505d4fab52 100644 --- a/x-pack/legacy/plugins/infra/public/components/formatted_time.tsx +++ b/x-pack/legacy/plugins/infra/public/components/formatted_time.tsx @@ -37,7 +37,6 @@ export const useFormattedTime = ( const dateFormat = formatMap[format]; const formattedTime = useMemo(() => getFormattedTime(time, dateFormat, fallbackFormat), [ - getFormattedTime, time, dateFormat, fallbackFormat, diff --git a/x-pack/legacy/plugins/infra/public/components/logging/log_entry_flyout/log_entry_actions_menu.tsx b/x-pack/legacy/plugins/infra/public/components/logging/log_entry_flyout/log_entry_actions_menu.tsx index 92c6ddd193609..d018b3a0f38ff 100644 --- a/x-pack/legacy/plugins/infra/public/components/logging/log_entry_flyout/log_entry_actions_menu.tsx +++ b/x-pack/legacy/plugins/infra/public/components/logging/log_entry_flyout/log_entry_actions_menu.tsx @@ -51,7 +51,7 @@ export const LogEntryActionsMenu: React.FunctionComponent<{ /> , ], - [uptimeLink] + [apmLink, uptimeLink] ); const hasMenuItems = useMemo(() => menuItems.length > 0, [menuItems]); diff --git a/x-pack/legacy/plugins/infra/public/components/logging/log_text_stream/text_styles.tsx b/x-pack/legacy/plugins/infra/public/components/logging/log_text_stream/text_styles.tsx index 1d40c88f5d1d0..e95ac6aa7923b 100644 --- a/x-pack/legacy/plugins/infra/public/components/logging/log_text_stream/text_styles.tsx +++ b/x-pack/legacy/plugins/infra/public/components/logging/log_text_stream/text_styles.tsx @@ -63,7 +63,7 @@ export const useMeasuredCharacterDimensions = (scale: TextScale) => { X ), - [scale] + [measureElement, scale] ); return { diff --git a/x-pack/legacy/plugins/infra/public/components/saved_views/create_modal.tsx b/x-pack/legacy/plugins/infra/public/components/saved_views/create_modal.tsx index 2fc96fbd22a9e..e52bb0653ecd3 100644 --- a/x-pack/legacy/plugins/infra/public/components/saved_views/create_modal.tsx +++ b/x-pack/legacy/plugins/infra/public/components/saved_views/create_modal.tsx @@ -36,7 +36,7 @@ export const SavedViewCreateModal = ({ close, save }: Props) => { const saveView = useCallback(() => { save(viewName, includeTime); close(); - }, [viewName, includeTime]); + }, [close, includeTime, save, viewName]); return ( diff --git a/x-pack/legacy/plugins/infra/public/components/source_configuration/add_log_column_popover.tsx b/x-pack/legacy/plugins/infra/public/components/source_configuration/add_log_column_popover.tsx index 9b83f62e7856b..fc8407c5298e6 100644 --- a/x-pack/legacy/plugins/infra/public/components/source_configuration/add_log_column_popover.tsx +++ b/x-pack/legacy/plugins/infra/public/components/source_configuration/add_log_column_popover.tsx @@ -94,7 +94,7 @@ export const AddLogColumnButtonAndPopover: React.FunctionComponent<{ addLogColumn(selectedOption.columnConfiguration); }, - [addLogColumn, availableColumnOptions] + [addLogColumn, availableColumnOptions, closePopover] ); return ( diff --git a/x-pack/legacy/plugins/infra/public/components/source_configuration/source_configuration_form_state.tsx b/x-pack/legacy/plugins/infra/public/components/source_configuration/source_configuration_form_state.tsx index 3614a88c1e99e..262649e20709b 100644 --- a/x-pack/legacy/plugins/infra/public/components/source_configuration/source_configuration_form_state.tsx +++ b/x-pack/legacy/plugins/infra/public/components/source_configuration/source_configuration_form_state.tsx @@ -52,7 +52,7 @@ export const useSourceConfigurationFormState = (configuration?: SourceConfigurat const resetForm = useCallback(() => { indicesConfigurationFormState.resetForm(); logColumnsConfigurationFormState.resetForm(); - }, [indicesConfigurationFormState.resetForm, logColumnsConfigurationFormState.formState]); + }, [indicesConfigurationFormState, logColumnsConfigurationFormState]); const isFormDirty = useMemo( () => indicesConfigurationFormState.isFormDirty || logColumnsConfigurationFormState.isFormDirty, diff --git a/x-pack/legacy/plugins/infra/public/components/waffle/waffle_inventory_switcher.tsx b/x-pack/legacy/plugins/infra/public/components/waffle/waffle_inventory_switcher.tsx index 38e87038b7c4f..c8f03cef4d6ac 100644 --- a/x-pack/legacy/plugins/infra/public/components/waffle/waffle_inventory_switcher.tsx +++ b/x-pack/legacy/plugins/infra/public/components/waffle/waffle_inventory_switcher.tsx @@ -17,28 +17,33 @@ import { } from '../../graphql/types'; import { findInventoryModel } from '../../../common/inventory_models'; -interface Props { +interface WaffleInventorySwitcherProps { nodeType: InfraNodeType; changeNodeType: (nodeType: InfraNodeType) => void; changeGroupBy: (groupBy: InfraSnapshotGroupbyInput[]) => void; changeMetric: (metric: InfraSnapshotMetricInput) => void; } -export const WaffleInventorySwitcher = (props: Props) => { +export const WaffleInventorySwitcher: React.FC = ({ + changeNodeType, + changeGroupBy, + changeMetric, + nodeType, +}) => { const [isOpen, setIsOpen] = useState(false); const closePopover = useCallback(() => setIsOpen(false), []); const openPopover = useCallback(() => setIsOpen(true), []); const goToNodeType = useCallback( - (nodeType: InfraNodeType) => { + (targetNodeType: InfraNodeType) => { closePopover(); - props.changeNodeType(nodeType); - props.changeGroupBy([]); - const inventoryModel = findInventoryModel(nodeType); - props.changeMetric({ + changeNodeType(targetNodeType); + changeGroupBy([]); + const inventoryModel = findInventoryModel(targetNodeType); + changeMetric({ type: inventoryModel.metrics.defaultSnapshot as InfraSnapshotMetricType, }); }, - [props.changeGroupBy, props.changeNodeType, props.changeMetric] + [closePopover, changeNodeType, changeGroupBy, changeMetric] ); const goToHost = useCallback(() => goToNodeType('host' as InfraNodeType), [goToNodeType]); const goToK8 = useCallback(() => goToNodeType('pod' as InfraNodeType), [goToNodeType]); @@ -68,10 +73,10 @@ export const WaffleInventorySwitcher = (props: Props) => { ], }, ], - [] + [goToDocker, goToHost, goToK8] ); const selectedText = useMemo(() => { - switch (props.nodeType) { + switch (nodeType) { case InfraNodeType.host: return i18n.translate('xpack.infra.waffle.nodeTypeSwitcher.hostsLabel', { defaultMessage: 'Hosts', @@ -81,7 +86,7 @@ export const WaffleInventorySwitcher = (props: Props) => { case InfraNodeType.container: return 'Docker'; } - }, [props.nodeType]); + }, [nodeType]); return ( diff --git a/x-pack/legacy/plugins/infra/public/containers/logs/log_analysis/log_analysis_capabilities.tsx b/x-pack/legacy/plugins/infra/public/containers/logs/log_analysis/log_analysis_capabilities.tsx index 7ac7d051e6783..360527af0fe40 100644 --- a/x-pack/legacy/plugins/infra/public/containers/logs/log_analysis/log_analysis_capabilities.tsx +++ b/x-pack/legacy/plugins/infra/public/containers/logs/log_analysis/log_analysis_capabilities.tsx @@ -46,7 +46,7 @@ export const useLogAnalysisCapabilities = () => { useEffect(() => { fetchMlCapabilities(); - }, []); + }, [fetchMlCapabilities]); const isLoading = useMemo(() => fetchMlCapabilitiesRequest.state === 'pending', [ fetchMlCapabilitiesRequest.state, diff --git a/x-pack/legacy/plugins/infra/public/containers/logs/log_analysis/log_analysis_jobs.tsx b/x-pack/legacy/plugins/infra/public/containers/logs/log_analysis/log_analysis_jobs.tsx index 0f386f416b866..8ebb2b674d72b 100644 --- a/x-pack/legacy/plugins/infra/public/containers/logs/log_analysis/log_analysis_jobs.tsx +++ b/x-pack/legacy/plugins/infra/public/containers/logs/log_analysis/log_analysis_jobs.tsx @@ -114,7 +114,7 @@ export const useLogAnalysisJobs = ({ const viewResults = useCallback(() => { dispatch({ type: 'viewedResults' }); - }, []); + }, [dispatch]); const cleanupAndSetup = useCallback( (indices: string[], start: number | undefined, end: number | undefined) => { @@ -127,16 +127,16 @@ export const useLogAnalysisJobs = ({ dispatch({ type: 'failedSetup' }); }); }, - [cleanupMLResources, setupMlModule] + [cleanupMLResources, dispatch, setupMlModule] ); const viewSetupForReconfiguration = useCallback(() => { dispatch({ type: 'requestedJobConfigurationUpdate' }); - }, []); + }, [dispatch]); const viewSetupForUpdate = useCallback(() => { dispatch({ type: 'requestedJobDefinitionUpdate' }); - }, []); + }, [dispatch]); useEffect(() => { fetchModuleDefinition(); diff --git a/x-pack/legacy/plugins/infra/public/containers/logs/log_analysis/log_analysis_results.tsx b/x-pack/legacy/plugins/infra/public/containers/logs/log_analysis/log_analysis_results.tsx index 81a80fb565a4b..bd76eae3a7058 100644 --- a/x-pack/legacy/plugins/infra/public/containers/logs/log_analysis/log_analysis_results.tsx +++ b/x-pack/legacy/plugins/infra/public/containers/logs/log_analysis/log_analysis_results.tsx @@ -50,7 +50,7 @@ export const useLogAnalysisResults = ({ useEffect(() => { getLogEntryRate(); - }, [sourceId, startTime, endTime, bucketDuration, lastRequestTime]); + }, [bucketDuration, endTime, getLogEntryRate, lastRequestTime, sourceId, startTime]); const logRateResults: LogRateResults | null = useMemo(() => { if (logEntryRate) { diff --git a/x-pack/legacy/plugins/infra/public/containers/logs/log_highlights/log_entry_highlights.tsx b/x-pack/legacy/plugins/infra/public/containers/logs/log_highlights/log_entry_highlights.tsx index 6ead866fb960a..2b19958a9b1a1 100644 --- a/x-pack/legacy/plugins/infra/public/containers/logs/log_highlights/log_entry_highlights.tsx +++ b/x-pack/legacy/plugins/infra/public/containers/logs/log_highlights/log_entry_highlights.tsx @@ -78,7 +78,7 @@ export const useLogEntryHighlights = ( } else { setLogEntryHighlights([]); } - }, [highlightTerms, startKey, endKey, filterQuery, sourceVersion]); + }, [endKey, filterQuery, highlightTerms, loadLogEntryHighlights, sourceVersion, startKey]); const logEntryHighlightsById = useMemo( () => diff --git a/x-pack/legacy/plugins/infra/public/containers/logs/log_highlights/log_summary_highlights.ts b/x-pack/legacy/plugins/infra/public/containers/logs/log_highlights/log_summary_highlights.ts index 34c66afda010e..874c70e016496 100644 --- a/x-pack/legacy/plugins/infra/public/containers/logs/log_highlights/log_summary_highlights.ts +++ b/x-pack/legacy/plugins/infra/public/containers/logs/log_highlights/log_summary_highlights.ts @@ -74,7 +74,15 @@ export const useLogSummaryHighlights = ( } else { setLogSummaryHighlights([]); } - }, [highlightTerms, start, end, bucketSize, filterQuery, sourceVersion]); + }, [ + bucketSize, + debouncedLoadSummaryHighlights, + end, + filterQuery, + highlightTerms, + sourceVersion, + start, + ]); return { logSummaryHighlights, From db84a98acc0cfc281c8cfb0c06e30ba897712ec8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Tue, 3 Dec 2019 12:00:31 +0100 Subject: [PATCH 06/20] Fix conditional hook usage and ineffective memoization --- .../pages/metrics/components/sub_section.tsx | 36 +++++++++---------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/x-pack/legacy/plugins/infra/public/pages/metrics/components/sub_section.tsx b/x-pack/legacy/plugins/infra/public/pages/metrics/components/sub_section.tsx index f3db3b1670199..325d510293135 100644 --- a/x-pack/legacy/plugins/infra/public/pages/metrics/components/sub_section.tsx +++ b/x-pack/legacy/plugins/infra/public/pages/metrics/components/sub_section.tsx @@ -23,29 +23,25 @@ export const SubSection: FunctionComponent = ({ isLiveStreaming, stopLiveStreaming, }) => { - if (!children || !metrics) { + const metric = useMemo(() => metrics?.find(m => m.id === id), [id, metrics]); + + if (!children || !metric) { return null; } - const metric = metrics.find(m => m.id === id); - if (!metric) { + + const childrenWithProps = Children.map(children, child => { + if (isValidElement(child)) { + return cloneElement(child, { + metric, + id, + onChangeRangeTime, + isLiveStreaming, + stopLiveStreaming, + }); + } return null; - } - const childrenWithProps = useMemo( - () => - Children.map(children, child => { - if (isValidElement(child)) { - return cloneElement(child, { - metric, - id, - onChangeRangeTime, - isLiveStreaming, - stopLiveStreaming, - }); - } - return null; - }), - [children, metric, id, onChangeRangeTime, isLiveStreaming, stopLiveStreaming] - ); + }); + return (
{label ? ( From 8a58208d20b182904914165b5b5f80e61e047ae5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Tue, 3 Dec 2019 12:45:32 +0100 Subject: [PATCH 07/20] Fix dangerous array mutation of useMemo closure --- .../pages/metrics/components/section.tsx | 71 ++++++++++--------- 1 file changed, 39 insertions(+), 32 deletions(-) diff --git a/x-pack/legacy/plugins/infra/public/pages/metrics/components/section.tsx b/x-pack/legacy/plugins/infra/public/pages/metrics/components/section.tsx index 32d2e2eff8ab9..2f9ed9f54df82 100644 --- a/x-pack/legacy/plugins/infra/public/pages/metrics/components/section.tsx +++ b/x-pack/legacy/plugins/infra/public/pages/metrics/components/section.tsx @@ -4,15 +4,15 @@ * you may not use this file except in compliance with the Elastic License. */ +import { EuiTitle } from '@elastic/eui'; import React, { - useContext, Children, - isValidElement, cloneElement, FunctionComponent, - useMemo, + isValidElement, + useContext, } from 'react'; -import { EuiTitle } from '@elastic/eui'; + import { SideNavContext, SubNavItem } from '../lib/side_nav_context'; import { LayoutProps } from '../types'; @@ -31,35 +31,42 @@ export const Section: FunctionComponent = ({ stopLiveStreaming, }) => { const { addNavItem } = useContext(SideNavContext); - const subNavItems: SubNavItem[] = []; - const childrenWithProps = useMemo( - () => - Children.map(children, child => { - if (isValidElement(child)) { - const metric = (metrics && metrics.find(m => m.id === child.props.id)) || null; - if (metric) { - subNavItems.push({ - id: child.props.id, - name: child.props.label, - onClick: () => { - const el = document.getElementById(child.props.id); - if (el) { - el.scrollIntoView(); - } - }, - }); - } - return cloneElement(child, { - metrics, - onChangeRangeTime, - isLiveStreaming, - stopLiveStreaming, - }); - } - return null; - }), - [children, metrics, onChangeRangeTime, isLiveStreaming, stopLiveStreaming] + const subNavItems = Children.toArray(children).reduce( + (accumulatedChildren, child) => { + if (!isValidElement(child)) { + return accumulatedChildren; + } + const metric = metrics?.find(m => m.id === child.props.id) ?? null; + if (metric === null) { + return accumulatedChildren; + } + return [ + ...accumulatedChildren, + { + id: child.props.id, + name: child.props.label, + onClick: () => { + const el = document.getElementById(child.props.id); + if (el) { + el.scrollIntoView(); + } + }, + }, + ]; + }, + [] + ); + + const childrenWithProps = Children.map(children, child => + isValidElement(child) + ? cloneElement(child, { + metrics, + onChangeRangeTime, + isLiveStreaming, + stopLiveStreaming, + }) + : null ); if (metrics && subNavItems.length) { From 3ffbfdff64ef607eb687ec01a199956fb4e88702 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Tue, 3 Dec 2019 12:46:46 +0100 Subject: [PATCH 08/20] Fix incorrect memoization dependencies --- .../pages/metrics/containers/with_metrics_time.tsx | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/x-pack/legacy/plugins/infra/public/pages/metrics/containers/with_metrics_time.tsx b/x-pack/legacy/plugins/infra/public/pages/metrics/containers/with_metrics_time.tsx index 6a89e75679468..f61fd5ee735a1 100644 --- a/x-pack/legacy/plugins/infra/public/pages/metrics/containers/with_metrics_time.tsx +++ b/x-pack/legacy/plugins/infra/public/pages/metrics/containers/with_metrics_time.tsx @@ -42,19 +42,15 @@ export const useMetricsTime = () => { interval: '>=1m', }); - const parsedFrom = dateMath.parse(timeRange.from); - const parsedTo = dateMath.parse(timeRange.to, { roundUp: true }); const parsedTimeRange = useMemo( () => ({ ...timeRange, - from: - (parsedFrom && parsedFrom.valueOf()) || - moment() - .subtract(1, 'hour') - .valueOf(), - to: (parsedTo && parsedTo.valueOf()) || moment().valueOf(), + from: (dateMath.parse(timeRange.from) ?? moment().subtract(1, 'hour'))?.valueOf(), + to: (dateMath.parse(timeRange.to, { roundUp: true }) ?? moment())?.valueOf(), }), - [parsedFrom, parsedTo, lastRefresh] + // lastRefresh is used to force a refresh for identical dates + // eslint-disable-next-line react-hooks/exhaustive-deps + [lastRefresh, timeRange] ); return { From fadb8baebfc9dfe2e639919d9cae76f4320b3524 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Tue, 3 Dec 2019 12:47:26 +0100 Subject: [PATCH 09/20] Whitelist externally managed dependency lists --- x-pack/legacy/plugins/infra/public/utils/cancellable_effect.ts | 3 +++ .../legacy/plugins/infra/public/utils/use_tracked_promise.ts | 2 ++ 2 files changed, 5 insertions(+) diff --git a/x-pack/legacy/plugins/infra/public/utils/cancellable_effect.ts b/x-pack/legacy/plugins/infra/public/utils/cancellable_effect.ts index bb7d253ea1557..a986af07f0c9a 100644 --- a/x-pack/legacy/plugins/infra/public/utils/cancellable_effect.ts +++ b/x-pack/legacy/plugins/infra/public/utils/cancellable_effect.ts @@ -27,5 +27,8 @@ export const useCancellableEffect = ( effect(() => cancellationSignal.isCancelled); return cancellationSignal.cancel; + + // the dependencies are managed externally + // eslint-disable-next-line react-hooks/exhaustive-deps }, deps); }; diff --git a/x-pack/legacy/plugins/infra/public/utils/use_tracked_promise.ts b/x-pack/legacy/plugins/infra/public/utils/use_tracked_promise.ts index 366caf0dfb156..c23bab7026aaa 100644 --- a/x-pack/legacy/plugins/infra/public/utils/use_tracked_promise.ts +++ b/x-pack/legacy/plugins/infra/public/utils/use_tracked_promise.ts @@ -190,6 +190,8 @@ export const useTrackedPromise = ( return newPendingPromise.promise; }, + // the dependencies are managed by the caller + // eslint-disable-next-line react-hooks/exhaustive-deps dependencies ); From c53a1336af5a576a1823bc336fa80d86bdefacd4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Tue, 3 Dec 2019 12:47:58 +0100 Subject: [PATCH 10/20] Fix incomplete dependency lists --- .../plugins/infra/public/utils/use_kibana_ui_setting.ts | 7 ++++++- .../plugins/infra/public/utils/use_visibility_state.ts | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/x-pack/legacy/plugins/infra/public/utils/use_kibana_ui_setting.ts b/x-pack/legacy/plugins/infra/public/utils/use_kibana_ui_setting.ts index c48f95a6521cf..1b08fb4231243 100644 --- a/x-pack/legacy/plugins/infra/public/utils/use_kibana_ui_setting.ts +++ b/x-pack/legacy/plugins/infra/public/utils/use_kibana_ui_setting.ts @@ -28,10 +28,15 @@ import { useObservable } from './use_observable'; export const useKibanaUiSetting = (key: string, defaultValue?: any) => { const uiSettingsClient = npSetup.core.uiSettings; - const uiSetting$ = useMemo(() => uiSettingsClient.get$(key, defaultValue), [uiSettingsClient]); + const uiSetting$ = useMemo(() => uiSettingsClient.get$(key, defaultValue), [ + defaultValue, + key, + uiSettingsClient, + ]); const uiSetting = useObservable(uiSetting$); const setUiSetting = useCallback((value: any) => uiSettingsClient.set(key, value), [ + key, uiSettingsClient, ]); diff --git a/x-pack/legacy/plugins/infra/public/utils/use_visibility_state.ts b/x-pack/legacy/plugins/infra/public/utils/use_visibility_state.ts index 5763834b1cc2a..f4d8b572e4f7f 100644 --- a/x-pack/legacy/plugins/infra/public/utils/use_visibility_state.ts +++ b/x-pack/legacy/plugins/infra/public/utils/use_visibility_state.ts @@ -20,6 +20,6 @@ export const useVisibilityState = (initialState: boolean) => { show, toggle, }), - [isVisible, show, hide] + [hide, isVisible, show, toggle] ); }; From 873031a5c13b9c27b70e53ab1e6c35a6d05ec228 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Tue, 3 Dec 2019 13:02:02 +0100 Subject: [PATCH 11/20] Fix conditional hook usage --- .../metrics/components/chart_section_vis.tsx | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/x-pack/legacy/plugins/infra/public/pages/metrics/components/chart_section_vis.tsx b/x-pack/legacy/plugins/infra/public/pages/metrics/components/chart_section_vis.tsx index 425b5a43f793f..309961cc39025 100644 --- a/x-pack/legacy/plugins/infra/public/pages/metrics/components/chart_section_vis.tsx +++ b/x-pack/legacy/plugins/infra/public/pages/metrics/components/chart_section_vis.tsx @@ -3,7 +3,7 @@ * or more contributor license agreements. Licensed under the Elastic License; * you may not use this file except in compliance with the Elastic License. */ -import React, { useCallback } from 'react'; +import React, { useCallback, useMemo } from 'react'; import moment from 'moment'; import { i18n } from '@kbn/i18n'; import { @@ -42,15 +42,15 @@ export const ChartSectionVis = ({ seriesOverrides, type, }: VisSectionProps) => { - if (!metric || !id) { - return null; - } const [dateFormat] = useKibanaUiSetting('dateFormat'); const valueFormatter = useCallback(getFormatter(formatter, formatterTemplate), [ formatter, formatterTemplate, ]); - const dateFormatter = useCallback(niceTimeFormatter(getMaxMinTimestamp(metric)), [metric]); + const dateFormatter = useMemo( + () => (metric != null ? niceTimeFormatter(getMaxMinTimestamp(metric)) : undefined), + [metric] + ); const handleTimeChange = useCallback( (from: number, to: number) => { if (onChangeRangeTime) { @@ -73,7 +73,9 @@ export const ChartSectionVis = ({ ), }; - if (!metric) { + if (!id) { + return null; + } else if (!metric) { return ( ); - } - - if (metric.series.some(seriesHasLessThen2DataPoints)) { + } else if (metric.series.some(seriesHasLessThen2DataPoints)) { return ( Date: Tue, 3 Dec 2019 13:06:01 +0100 Subject: [PATCH 12/20] Fix conditional hook usage --- .../infra/public/pages/infrastructure/index.tsx | 15 ++++++++++----- .../infrastructure/metrics_explorer/index.tsx | 7 +------ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/x-pack/legacy/plugins/infra/public/pages/infrastructure/index.tsx b/x-pack/legacy/plugins/infra/public/pages/infrastructure/index.tsx index fe48fcc62f77d..9efbbe790abc1 100644 --- a/x-pack/legacy/plugins/infra/public/pages/infrastructure/index.tsx +++ b/x-pack/legacy/plugins/infra/public/pages/infrastructure/index.tsx @@ -24,6 +24,7 @@ import { MetricsExplorerPage } from './metrics_explorer'; import { SnapshotPage } from './snapshot'; import { SettingsPage } from '../shared/settings'; import { AppNavigation } from '../../components/navigation/app_navigation'; +import { SourceLoadingPage } from '../../components/source_loading_page'; interface InfrastructurePageProps extends RouteComponentProps { uiCapabilities: UICapabilities; @@ -95,11 +96,15 @@ export const InfrastructurePage = injectUICapabilities( {({ configuration, createDerivedIndexPattern }) => ( - + {configuration ? ( + + ) : ( + + )} )} diff --git a/x-pack/legacy/plugins/infra/public/pages/infrastructure/metrics_explorer/index.tsx b/x-pack/legacy/plugins/infra/public/pages/infrastructure/metrics_explorer/index.tsx index 63f5a81967618..4db4319b91d3c 100644 --- a/x-pack/legacy/plugins/infra/public/pages/infrastructure/metrics_explorer/index.tsx +++ b/x-pack/legacy/plugins/infra/public/pages/infrastructure/metrics_explorer/index.tsx @@ -11,22 +11,17 @@ import { IIndexPattern } from 'src/plugins/data/public'; import { DocumentTitle } from '../../../components/document_title'; import { MetricsExplorerCharts } from '../../../components/metrics_explorer/charts'; import { MetricsExplorerToolbar } from '../../../components/metrics_explorer/toolbar'; -import { SourceLoadingPage } from '../../../components/source_loading_page'; import { SourceQuery } from '../../../../common/graphql/types'; import { NoData } from '../../../components/empty_states'; import { useMetricsExplorerState } from './use_metric_explorer_state'; import { useTrackPageview } from '../../../hooks/use_track_metric'; interface MetricsExplorerPageProps { - source: SourceQuery.Query['source']['configuration'] | undefined; + source: SourceQuery.Query['source']['configuration']; derivedIndexPattern: IIndexPattern; } export const MetricsExplorerPage = ({ source, derivedIndexPattern }: MetricsExplorerPageProps) => { - if (!source) { - return ; - } - const { loading, error, From 35a09022ec6de181b652c0dd23b687a87fd4a700 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Tue, 3 Dec 2019 13:08:07 +0100 Subject: [PATCH 13/20] Whitelist external dependencies --- x-pack/legacy/plugins/infra/public/hooks/use_track_metric.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/x-pack/legacy/plugins/infra/public/hooks/use_track_metric.tsx b/x-pack/legacy/plugins/infra/public/hooks/use_track_metric.tsx index 379b3af3f1063..c5945ab808202 100644 --- a/x-pack/legacy/plugins/infra/public/hooks/use_track_metric.tsx +++ b/x-pack/legacy/plugins/infra/public/hooks/use_track_metric.tsx @@ -57,6 +57,9 @@ export function useTrackMetric( const trackUiMetric = getTrackerForApp(app); const id = setTimeout(() => trackUiMetric(metricType, decoratedMetric), Math.max(delay, 0)); return () => clearTimeout(id); + + // the dependencies are managed externally + // eslint-disable-next-line react-hooks/exhaustive-deps }, effectDependencies); } From 8a851244d2f2d27195cb7b0a916c159364375385 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Tue, 3 Dec 2019 13:17:46 +0100 Subject: [PATCH 14/20] Fix incomplete dependency lists --- .../use_metrics_explorer_options.ts | 2 +- .../infra/public/hooks/use_saved_view.ts | 26 +++++++++---------- .../use_metric_explorer_state.ts | 12 ++++----- .../pages/logs/analysis/page_content.tsx | 2 +- .../analysis/sections/anomalies/table.tsx | 2 +- .../analysis_setup_indices_form.tsx | 2 +- 6 files changed, 22 insertions(+), 24 deletions(-) diff --git a/x-pack/legacy/plugins/infra/public/containers/metrics_explorer/use_metrics_explorer_options.ts b/x-pack/legacy/plugins/infra/public/containers/metrics_explorer/use_metrics_explorer_options.ts index 278f3e0a9c17d..de7a8d5805ecc 100644 --- a/x-pack/legacy/plugins/infra/public/containers/metrics_explorer/use_metrics_explorer_options.ts +++ b/x-pack/legacy/plugins/infra/public/containers/metrics_explorer/use_metrics_explorer_options.ts @@ -102,7 +102,7 @@ function useStateWithLocalStorage( const [state, setState] = useState(parseJsonOrDefault(storageState, defaultState)); useEffect(() => { localStorage.setItem(key, JSON.stringify(state)); - }, [state]); + }, [key, state]); return [state, setState]; } diff --git a/x-pack/legacy/plugins/infra/public/hooks/use_saved_view.ts b/x-pack/legacy/plugins/infra/public/hooks/use_saved_view.ts index f3d4833a5a755..0b7b4a4ebf04d 100644 --- a/x-pack/legacy/plugins/infra/public/hooks/use_saved_view.ts +++ b/x-pack/legacy/plugins/infra/public/hooks/use_saved_view.ts @@ -26,10 +26,10 @@ export const useSavedView = (defaultViewState: ViewState, viewType: s >(viewType); const { create, error: errorOnCreate } = useCreateSavedObject(viewType); const { deleteObject, deletedId } = useDeleteSavedObject(viewType); - const deleteView = useCallback((id: string) => deleteObject(id), []); - const saveView = useCallback((d: { [p: string]: any }) => create(d), []); + const deleteView = useCallback((id: string) => deleteObject(id), [deleteObject]); + const saveView = useCallback((d: { [p: string]: any }) => create(d), [create]); - const savedObjects = data ? data.savedObjects : []; + const savedObjects = useMemo(() => (data ? data.savedObjects : []), [data]); const views = useMemo(() => { const items: Array> = [ { @@ -42,19 +42,17 @@ export const useSavedView = (defaultViewState: ViewState, viewType: s }, ]; - if (data) { - data.savedObjects.forEach( - o => - o.type === viewType && - items.push({ - ...o.attributes, - id: o.id, - }) - ); - } + savedObjects.forEach( + o => + o.type === viewType && + items.push({ + ...o.attributes, + id: o.id, + }) + ); return items; - }, [savedObjects, defaultViewState]); + }, [defaultViewState, savedObjects, viewType]); return { views, diff --git a/x-pack/legacy/plugins/infra/public/pages/infrastructure/metrics_explorer/use_metric_explorer_state.ts b/x-pack/legacy/plugins/infra/public/pages/infrastructure/metrics_explorer/use_metric_explorer_state.ts index 415a6ae89a8b1..57ea886169701 100644 --- a/x-pack/legacy/plugins/infra/public/pages/infrastructure/metrics_explorer/use_metric_explorer_state.ts +++ b/x-pack/legacy/plugins/infra/public/pages/infrastructure/metrics_explorer/use_metric_explorer_state.ts @@ -59,7 +59,7 @@ export const useMetricsExplorerState = ( setAfterKey(null); setTimeRange({ ...currentTimerange, from: start, to: end }); }, - [currentTimerange] + [currentTimerange, setTimeRange] ); const handleGroupByChange = useCallback( @@ -70,7 +70,7 @@ export const useMetricsExplorerState = ( groupBy: groupBy || void 0, }); }, - [options] + [options, setOptions] ); const handleFilterQuerySubmit = useCallback( @@ -81,7 +81,7 @@ export const useMetricsExplorerState = ( filterQuery: query, }); }, - [options] + [options, setOptions] ); const handleMetricsChange = useCallback( @@ -92,7 +92,7 @@ export const useMetricsExplorerState = ( metrics, }); }, - [options] + [options, setOptions] ); const handleAggregationChange = useCallback( @@ -109,7 +109,7 @@ export const useMetricsExplorerState = ( })); setOptions({ ...options, aggregation, metrics }); }, - [options] + [options, setOptions] ); const onViewStateChange = useCallback( @@ -124,7 +124,7 @@ export const useMetricsExplorerState = ( setOptions(vs.options); } }, - [setChartOptions, setTimeRange, setTimeRange] + [setChartOptions, setOptions, setTimeRange] ); return { diff --git a/x-pack/legacy/plugins/infra/public/pages/logs/analysis/page_content.tsx b/x-pack/legacy/plugins/infra/public/pages/logs/analysis/page_content.tsx index f0a26eae25ecb..d9944d1b8d521 100644 --- a/x-pack/legacy/plugins/infra/public/pages/logs/analysis/page_content.tsx +++ b/x-pack/legacy/plugins/infra/public/pages/logs/analysis/page_content.tsx @@ -33,7 +33,7 @@ export const AnalysisPageContent = () => { useEffect(() => { fetchJobStatus(); - }, []); + }, [fetchJobStatus]); if (!hasLogAnalysisCapabilites) { return ; diff --git a/x-pack/legacy/plugins/infra/public/pages/logs/analysis/sections/anomalies/table.tsx b/x-pack/legacy/plugins/infra/public/pages/logs/analysis/sections/anomalies/table.tsx index c0016d07c290b..5d8e6262ddf28 100644 --- a/x-pack/legacy/plugins/infra/public/pages/logs/analysis/sections/anomalies/table.tsx +++ b/x-pack/legacy/plugins/infra/public/pages/logs/analysis/sections/anomalies/table.tsx @@ -124,7 +124,7 @@ export const AnomaliesTable: React.FunctionComponent<{ setItemIdToExpandedRowMap(newItemIdToExpandedRowMap); } }, - [results, setTimeRange, timeRange, itemIdToExpandedRowMap, setItemIdToExpandedRowMap] + [itemIdToExpandedRowMap, jobId, results, setTimeRange, timeRange] ); const columns = [ diff --git a/x-pack/legacy/plugins/infra/public/pages/logs/analysis/setup/initial_configuration_step/analysis_setup_indices_form.tsx b/x-pack/legacy/plugins/infra/public/pages/logs/analysis/setup/initial_configuration_step/analysis_setup_indices_form.tsx index 585a65b9ad1c8..c34a4dd203659 100644 --- a/x-pack/legacy/plugins/infra/public/pages/logs/analysis/setup/initial_configuration_step/analysis_setup_indices_form.tsx +++ b/x-pack/legacy/plugins/infra/public/pages/logs/analysis/setup/initial_configuration_step/analysis_setup_indices_form.tsx @@ -55,7 +55,7 @@ export const AnalysisSetupIndicesForm: React.FunctionComponent<{
); }), - [indices] + [handleCheckboxChange, indices] ); return ( From 5e523b63949ebcfe3e34b82cbacc72ec78f0bb61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Tue, 3 Dec 2019 13:18:28 +0100 Subject: [PATCH 15/20] Temporarily whitelist complex effect dependency list --- .../containers/metrics_explorer/use_metrics_explorer_data.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/x-pack/legacy/plugins/infra/public/containers/metrics_explorer/use_metrics_explorer_data.ts b/x-pack/legacy/plugins/infra/public/containers/metrics_explorer/use_metrics_explorer_data.ts index 1418d6aef67ac..c2a599ea1ae78 100644 --- a/x-pack/legacy/plugins/infra/public/containers/metrics_explorer/use_metrics_explorer_data.ts +++ b/x-pack/legacy/plugins/infra/public/containers/metrics_explorer/use_metrics_explorer_data.ts @@ -96,6 +96,9 @@ export function useMetricsExplorerData( } setLoading(false); })(); + + // TODO: fix this dependency list while preserving the semantics + // eslint-disable-next-line react-hooks/exhaustive-deps }, [options, source, timerange, signal, afterKey]); return { error, loading, data }; } From 61d70952ffc5c30b6cc2fdfb35224fe148169c4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Tue, 3 Dec 2019 13:27:33 +0100 Subject: [PATCH 16/20] Avoid useEffect for initialization --- .../public/containers/logs/with_stream_items.ts | 14 ++++++++------ .../public/pages/logs/stream/page_logs_content.tsx | 9 ++------- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/x-pack/legacy/plugins/infra/public/containers/logs/with_stream_items.ts b/x-pack/legacy/plugins/infra/public/containers/logs/with_stream_items.ts index 12117d88f8283..b944bad0bff42 100644 --- a/x-pack/legacy/plugins/infra/public/containers/logs/with_stream_items.ts +++ b/x-pack/legacy/plugins/infra/public/containers/logs/with_stream_items.ts @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import { useEffect, useContext, useMemo } from 'react'; +import { useContext, useEffect, useMemo, useState } from 'react'; import { connect } from 'react-redux'; import { StreamItem, LogEntryStreamItem } from '../../components/logging/log_text_stream/item'; @@ -52,6 +52,9 @@ export const WithStreamItems = withStreamItems( >; initializeOnMount: boolean; }) => { + const [shouldInitialize, setShouldInitialize] = useState( + initializeOnMount && !props.isReloading && !props.isLoadingMore + ); const { currentHighlightKey, logEntryHighlightsById } = useContext(LogHighlightsState.Context); const items = useMemo( () => @@ -70,11 +73,10 @@ export const WithStreamItems = withStreamItems( ] ); - useEffect(() => { - if (initializeOnMount && !props.isReloading && !props.isLoadingMore) { - props.reloadEntries(); - } - }, []); + if (shouldInitialize) { + setShouldInitialize(false); + props.reloadEntries(); + } return children({ ...props, diff --git a/x-pack/legacy/plugins/infra/public/pages/logs/stream/page_logs_content.tsx b/x-pack/legacy/plugins/infra/public/pages/logs/stream/page_logs_content.tsx index beb5eb391d368..f0f64489a7485 100644 --- a/x-pack/legacy/plugins/infra/public/pages/logs/stream/page_logs_content.tsx +++ b/x-pack/legacy/plugins/infra/public/pages/logs/stream/page_logs_content.tsx @@ -134,13 +134,8 @@ export const LogsPageLogsContent: React.FunctionComponent = () => { {({ buckets }) => ( - {({ - isAutoReloading, - jumpToTargetPosition, - visibleMidpointTime, - visibleTimeInterval, - }) => ( - + {({ jumpToTargetPosition, visibleMidpointTime, visibleTimeInterval }) => ( + {({ isReloading }) => ( Date: Tue, 3 Dec 2019 13:36:39 +0100 Subject: [PATCH 17/20] Fix incomplete dependency lists --- .../logs/log_analysis/log_analysis_setup_state.tsx | 2 +- .../containers/logs/log_highlights/next_and_previous.tsx | 2 +- .../containers/logs/log_highlights/redux_bridges.tsx | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/x-pack/legacy/plugins/infra/public/containers/logs/log_analysis/log_analysis_setup_state.tsx b/x-pack/legacy/plugins/infra/public/containers/logs/log_analysis/log_analysis_setup_state.tsx index c965c50bedccc..71555b077e0c4 100644 --- a/x-pack/legacy/plugins/infra/public/containers/logs/log_analysis/log_analysis_setup_state.tsx +++ b/x-pack/legacy/plugins/infra/public/containers/logs/log_analysis/log_analysis_setup_state.tsx @@ -122,7 +122,7 @@ export const useAnalysisSetupState = ({ return validatedIndices.reduce((errors, index) => { return selectedIndexNames.includes(index.index) ? errors.concat(index.errors) : errors; }, []); - }, [selectedIndexNames, validatedIndices, validateIndicesRequest.state]); + }, [isValidating, validateIndicesRequest.state, selectedIndexNames, validatedIndices]); return { cleanupAndSetup, diff --git a/x-pack/legacy/plugins/infra/public/containers/logs/log_highlights/next_and_previous.tsx b/x-pack/legacy/plugins/infra/public/containers/logs/log_highlights/next_and_previous.tsx index 95ead50119eb4..62a43a5412825 100644 --- a/x-pack/legacy/plugins/infra/public/containers/logs/log_highlights/next_and_previous.tsx +++ b/x-pack/legacy/plugins/infra/public/containers/logs/log_highlights/next_and_previous.tsx @@ -53,7 +53,7 @@ export const useNextAndPrevious = ({ const initialTimeKey = getUniqueLogEntryKey(entries[initialIndex]); setCurrentTimeKey(initialTimeKey); } - }, [currentTimeKey, entries, setCurrentTimeKey]); + }, [currentTimeKey, entries, setCurrentTimeKey, visibleMidpoint]); const indexOfCurrentTimeKey = useMemo(() => { if (currentTimeKey && entries.length > 0) { diff --git a/x-pack/legacy/plugins/infra/public/containers/logs/log_highlights/redux_bridges.tsx b/x-pack/legacy/plugins/infra/public/containers/logs/log_highlights/redux_bridges.tsx index 220eaade12fa6..159877c2fa6b8 100644 --- a/x-pack/legacy/plugins/infra/public/containers/logs/log_highlights/redux_bridges.tsx +++ b/x-pack/legacy/plugins/infra/public/containers/logs/log_highlights/redux_bridges.tsx @@ -20,7 +20,7 @@ export const LogHighlightsStreamItemsBridge = withStreamItems( useEffect(() => { setStartKey(entriesStart); setEndKey(entriesEnd); - }, [entriesStart, entriesEnd]); + }, [entriesStart, entriesEnd, setStartKey, setEndKey]); return null; } @@ -37,11 +37,11 @@ export const LogHighlightsPositionBridge = withLogPosition( const { setJumpToTarget, setVisibleMidpoint } = useContext(LogHighlightsState.Context); useEffect(() => { setVisibleMidpoint(visibleMidpoint); - }, [visibleMidpoint]); + }, [setVisibleMidpoint, visibleMidpoint]); useEffect(() => { setJumpToTarget(() => jumpToTargetPosition); - }, [jumpToTargetPosition]); + }, [jumpToTargetPosition, setJumpToTarget]); return null; } @@ -53,7 +53,7 @@ export const LogHighlightsFilterQueryBridge = withLogFilter( useEffect(() => { setFilterQuery(serializedFilterQuery); - }, [serializedFilterQuery]); + }, [serializedFilterQuery, setFilterQuery]); return null; } From 8f7f34bc8b3bc7fb45dcc7e1df3861dee9012962 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Tue, 3 Dec 2019 14:03:05 +0100 Subject: [PATCH 18/20] Wrap all imperative calls into useEffect --- .../containers/logs/with_stream_items.ts | 17 ++++++++------ .../infra/public/utils/use_url_state.ts | 22 ++++++++++--------- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/x-pack/legacy/plugins/infra/public/containers/logs/with_stream_items.ts b/x-pack/legacy/plugins/infra/public/containers/logs/with_stream_items.ts index b944bad0bff42..d84918028e105 100644 --- a/x-pack/legacy/plugins/infra/public/containers/logs/with_stream_items.ts +++ b/x-pack/legacy/plugins/infra/public/containers/logs/with_stream_items.ts @@ -52,9 +52,6 @@ export const WithStreamItems = withStreamItems( >; initializeOnMount: boolean; }) => { - const [shouldInitialize, setShouldInitialize] = useState( - initializeOnMount && !props.isReloading && !props.isLoadingMore - ); const { currentHighlightKey, logEntryHighlightsById } = useContext(LogHighlightsState.Context); const items = useMemo( () => @@ -73,10 +70,16 @@ export const WithStreamItems = withStreamItems( ] ); - if (shouldInitialize) { - setShouldInitialize(false); - props.reloadEntries(); - } + const [shouldInitialize, setShouldInitialize] = useState( + initializeOnMount && !props.isReloading && !props.isLoadingMore + ); + + useEffect(() => { + if (shouldInitialize) { + setShouldInitialize(false); + props.reloadEntries(); + } + }, [shouldInitialize, props]); return children({ ...props, diff --git a/x-pack/legacy/plugins/infra/public/utils/use_url_state.ts b/x-pack/legacy/plugins/infra/public/utils/use_url_state.ts index 5f107c99ae86e..79a5d552bcd78 100644 --- a/x-pack/legacy/plugins/infra/public/utils/use_url_state.ts +++ b/x-pack/legacy/plugins/infra/public/utils/use_url_state.ts @@ -5,10 +5,10 @@ */ import { Location } from 'history'; -import { useMemo, useCallback, useState } from 'react'; +import { useCallback, useEffect, useMemo, useState } from 'react'; import { decode, encode, RisonValue } from 'rison-node'; - import { QueryString } from 'ui/utils/query_string'; + import { useHistory } from './history_context'; export const useUrlState = ({ @@ -42,10 +42,6 @@ export const useUrlState = ({ urlStateString, ]); - const [shouldInitialize, setShouldInitialize] = useState( - writeDefaultState && typeof decodedState === 'undefined' - ); - const state = useMemo(() => (typeof decodedState !== 'undefined' ? decodedState : defaultState), [ defaultState, decodedState, @@ -74,10 +70,16 @@ export const useUrlState = ({ [encodeUrlState, history, urlStateKey] ); - if (shouldInitialize) { - setShouldInitialize(false); - setState(defaultState); - } + const [shouldInitialize, setShouldInitialize] = useState( + writeDefaultState && typeof decodedState === 'undefined' + ); + + useEffect(() => { + if (shouldInitialize) { + setShouldInitialize(false); + setState(defaultState); + } + }, [shouldInitialize, setState, defaultState]); return [state, setState] as [typeof state, typeof setState]; }; From 13b7e90dce780426e498bbe9e6e23c3b4f57b82e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Mon, 9 Dec 2019 16:33:23 +0100 Subject: [PATCH 19/20] Fix conditional hook usage --- .../metrics/components/node_details_page.tsx | 10 ++--- .../infra/public/pages/metrics/index.tsx | 42 ++++++++++--------- 2 files changed, 25 insertions(+), 27 deletions(-) diff --git a/x-pack/legacy/plugins/infra/public/pages/metrics/components/node_details_page.tsx b/x-pack/legacy/plugins/infra/public/pages/metrics/components/node_details_page.tsx index cc1f23d676448..933831c6ec87d 100644 --- a/x-pack/legacy/plugins/infra/public/pages/metrics/components/node_details_page.tsx +++ b/x-pack/legacy/plugins/infra/public/pages/metrics/components/node_details_page.tsx @@ -41,7 +41,7 @@ interface Props { isAutoReloading: boolean; refreshInterval: number; sideNav: NavItem[]; - metadata: InfraMetadata | null; + metadata: InfraMetadata; addNavItem(item: NavItem): void; setRefreshInterval(refreshInterval: number): void; setAutoReload(isAutoReloading: boolean): void; @@ -49,10 +49,6 @@ interface Props { setTimeRange(timeRange: MetricsTimeInput): void; } export const NodeDetailsPage = (props: Props) => { - if (!props.metadata) { - return null; - } - const { parsedTimeRange } = props; const { metrics, loading, makeRequest, error } = useNodeDetails( props.requiredMetrics, @@ -65,11 +61,11 @@ export const NodeDetailsPage = (props: Props) => { const refetch = useCallback(() => { makeRequest(); - }, []); + }, [makeRequest]); useEffect(() => { makeRequest(); - }, [parsedTimeRange]); + }, [makeRequest, parsedTimeRange]); if (error) { return ; diff --git a/x-pack/legacy/plugins/infra/public/pages/metrics/index.tsx b/x-pack/legacy/plugins/infra/public/pages/metrics/index.tsx index 93253406aec2d..b330ad02f1022 100644 --- a/x-pack/legacy/plugins/infra/public/pages/metrics/index.tsx +++ b/x-pack/legacy/plugins/infra/public/pages/metrics/index.tsx @@ -112,26 +112,28 @@ export const MetricDetail = withMetricPageProviders( })} /> - + {metadata ? ( + + ) : null} )} From 1efb71143f982abd1dcafd5ae1c655e754f448ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20St=C3=BCrmer?= Date: Tue, 10 Dec 2019 18:00:53 +0100 Subject: [PATCH 20/20] Name function more specifically --- .../public/components/metrics_explorer/metrics.tsx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/x-pack/legacy/plugins/infra/public/components/metrics_explorer/metrics.tsx b/x-pack/legacy/plugins/infra/public/components/metrics_explorer/metrics.tsx index 5cc7878a0c26e..42df7c6915a0d 100644 --- a/x-pack/legacy/plugins/infra/public/components/metrics_explorer/metrics.tsx +++ b/x-pack/legacy/plugins/infra/public/components/metrics_explorer/metrics.tsx @@ -34,10 +34,10 @@ export const MetricsExplorerMetrics = ({ options, onChange, fields, autoFocus = const [shouldFocus, setShouldFocus] = useState(autoFocus); // the EuiCombobox forwards the ref to an input element - const handleInputRef = useCallback( - (ref: HTMLInputElement | null) => { - if (ref && shouldFocus) { - ref.focus(); + const autoFocusInputElement = useCallback( + (inputElement: HTMLInputElement | null) => { + if (inputElement && shouldFocus) { + inputElement.focus(); setShouldFocus(false); } }, @@ -81,7 +81,7 @@ export const MetricsExplorerMetrics = ({ options, onChange, fields, autoFocus = selectedOptions={selectedOptions} onChange={handleChange} isClearable={true} - inputRef={handleInputRef} + inputRef={autoFocusInputElement} /> ); };