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

[Logs + Metrics UI] Remove eslint exceptions #50979

Merged
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
214c285
Remove infra eslint exception
weltenwort Nov 18, 2019
9aa42ed
Enhance useUrlState to write default to URL
weltenwort Dec 2, 2019
6a660d4
Refactor autoFocus ref handler to avoid useEffect
weltenwort Dec 2, 2019
4e0f0db
Refactor onChange callback to avoid useEffect
weltenwort Dec 2, 2019
c366a36
Fix incorrect dependency lists
weltenwort Dec 2, 2019
db84a98
Fix conditional hook usage and ineffective memoization
weltenwort Dec 3, 2019
8a58208
Fix dangerous array mutation of useMemo closure
weltenwort Dec 3, 2019
3ffbfdf
Fix incorrect memoization dependencies
weltenwort Dec 3, 2019
fadb8ba
Whitelist externally managed dependency lists
weltenwort Dec 3, 2019
c53a133
Fix incomplete dependency lists
weltenwort Dec 3, 2019
873031a
Fix conditional hook usage
weltenwort Dec 3, 2019
af5d428
Fix conditional hook usage
weltenwort Dec 3, 2019
35a0902
Whitelist external dependencies
weltenwort Dec 3, 2019
8a85124
Fix incomplete dependency lists
weltenwort Dec 3, 2019
5e523b6
Temporarily whitelist complex effect dependency list
weltenwort Dec 3, 2019
61d7095
Avoid useEffect for initialization
weltenwort Dec 3, 2019
35bf164
Fix incomplete dependency lists
weltenwort Dec 3, 2019
8f7f34b
Wrap all imperative calls into useEffect
weltenwort Dec 3, 2019
24270c8
Merge branch 'master' into logs-metrics-ui-remove-eslint-exceptions
weltenwort Dec 9, 2019
13b7e90
Fix conditional hook usage
weltenwort Dec 9, 2019
8e4fa68
Merge branch 'master' into logs-metrics-ui-remove-eslint-exceptions
weltenwort Dec 10, 2019
1efb711
Name function more specifically
weltenwort Dec 10, 2019
c4dd7d4
Merge branch 'master' into logs-metrics-ui-remove-eslint-exceptions
weltenwort Dec 11, 2019
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
7 changes: 0 additions & 7 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -170,13 +170,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: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ export const useFormattedTime = (

const dateFormat = formatMap[format];
const formattedTime = useMemo(() => getFormattedTime(time, dateFormat, fallbackFormat), [
getFormattedTime,
time,
dateFormat,
fallbackFormat,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export const LogEntryActionsMenu: React.FunctionComponent<{
/>
</EuiContextMenuItem>,
],
[uptimeLink]
[apmLink, uptimeLink]
);

const hasMenuItems = useMemo(() => menuItems.length > 0, [menuItems]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -47,8 +47,25 @@ export const LogHighlightsMenu: React.FC<LogHighlightsMenuProps> = ({
} = useVisibilityState(false);

// Input field state
const [highlightTerm, setHighlightTerm] = useState('');
const [highlightTerm, _setHighlightTerm] = useState('');

const debouncedOnChange = useMemo(() => debounce(onChange, 275), [onChange]);
const setHighlightTerm = useCallback<typeof _setHighlightTerm>(
valueOrUpdater =>
_setHighlightTerm(previousHighlightTerm => {
const newHighlightTerm =
typeof valueOrUpdater === 'function'
? valueOrUpdater(previousHighlightTerm)
: valueOrUpdater;

if (newHighlightTerm !== previousHighlightTerm) {
debouncedOnChange([newHighlightTerm]);
Copy link
Member Author

Choose a reason for hiding this comment

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

ℹ️ This makes useEffect unnecessary by moving the onChange call into the setter.

}

return newHighlightTerm;
}),
[debouncedOnChange]
);
const changeHighlightTerm = useCallback(
e => {
const value = e.target.value;
Expand All @@ -57,9 +74,6 @@ export const LogHighlightsMenu: React.FC<LogHighlightsMenuProps> = ({
[setHighlightTerm]
);
const clearHighlightTerm = useCallback(() => setHighlightTerm(''), [setHighlightTerm]);
useEffect(() => {
debouncedOnChange([highlightTerm]);
}, [highlightTerm]);

const button = (
<EuiButtonEmpty color="text" size="xs" iconType="brush" onClick={togglePopover}>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ export const useMeasuredCharacterDimensions = (scale: TextScale) => {
X
</MonospaceCharacterDimensionsProbe>
),
[scale]
[measureElement, scale]
);

return {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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<HTMLInputElement | null>(null);
const [focusOnce, setFocusState] = useState<boolean>(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(
Copy link
Member Author

Choose a reason for hiding this comment

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

ℹ️ Since the stored Ref was not used anywhere the imperative action can just be performed in the already imperative ref callback.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would rename it then to something on the line of focusInput()

(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(
Expand All @@ -59,7 +54,7 @@ export const MetricsExplorerMetrics = ({ options, onChange, fields, autoFocus =
}))
);
},
[options, onChange]
[onChange, options.aggregation, colors]
);

const comboOptions = fields
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export const SavedViewCreateModal = ({ close, save, isInvalid }: Props) => {

const saveView = useCallback(() => {
save(viewName, includeTime);
}, [viewName, includeTime]);
}, [includeTime, save, viewName]);

return (
<EuiOverlayMask>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ export const AddLogColumnButtonAndPopover: React.FunctionComponent<{

addLogColumn(selectedOption.columnConfiguration);
},
[addLogColumn, availableColumnOptions]
[addLogColumn, availableColumnOptions, closePopover]
);

return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<WaffleInventorySwitcherProps> = ({
changeNodeType,
changeGroupBy,
changeMetric,
nodeType,
}) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

ℹ️ This destructuring just allows for correct dependency declaration below.

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]);
Expand Down Expand Up @@ -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',
Expand All @@ -81,7 +86,7 @@ export const WaffleInventorySwitcher = (props: Props) => {
case InfraNodeType.container:
return 'Docker';
}
}, [props.nodeType]);
}, [nodeType]);

return (
<EuiFilterGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export const useLogAnalysisCapabilities = () => {

useEffect(() => {
fetchMlCapabilities();
}, []);
}, [fetchMlCapabilities]);

const isLoading = useMemo(() => fetchMlCapabilitiesRequest.state === 'pending', [
fetchMlCapabilitiesRequest.state,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand All @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
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 { 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([
Expand Down Expand Up @@ -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,
Copy link
Member Author

Choose a reason for hiding this comment

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

ℹ️ The initialization is now handled by the useUrlState hook itself.

});

useEffect(() => {
setTimeRange(timeRange);
}, []);

const [autoRefresh, setAutoRefresh] = useUrlState({
defaultState: {
isPaused: false,
Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ export const useAnalysisSetupState = ({
return validatedIndices.reduce<ValidationIndicesUIError[]>((errors, index) => {
return selectedIndexNames.includes(index.index) ? errors.concat(index.errors) : errors;
}, []);
}, [selectedIndexNames, validatedIndices, validateIndicesRequest.state]);
}, [isValidating, validateIndicesRequest.state, selectedIndexNames, validatedIndices]);

return {
cleanupAndSetup,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export const useLogEntryHighlights = (
} else {
setLogEntryHighlights([]);
}
}, [highlightTerms, startKey, endKey, filterQuery, sourceVersion]);
}, [endKey, filterQuery, highlightTerms, loadLogEntryHighlights, sourceVersion, startKey]);

const logEntryHighlightsById = useMemo(
() =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,15 @@ export const useLogSummaryHighlights = (
} else {
setLogSummaryHighlights([]);
}
}, [highlightTerms, start, end, bucketSize, filterQuery, sourceVersion]);
}, [
bucketSize,
debouncedLoadSummaryHighlights,
end,
filterQuery,
highlightTerms,
sourceVersion,
start,
]);

return {
logSummaryHighlights,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export const LogHighlightsStreamItemsBridge = withStreamItems(
useEffect(() => {
setStartKey(entriesStart);
setEndKey(entriesEnd);
}, [entriesStart, entriesEnd]);
}, [entriesStart, entriesEnd, setStartKey, setEndKey]);

return null;
}
Expand All @@ -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;
}
Expand All @@ -53,7 +53,7 @@ export const LogHighlightsFilterQueryBridge = withLogFilter(

useEffect(() => {
setFilterQuery(serializedFilterQuery);
}, [serializedFilterQuery]);
}, [serializedFilterQuery, setFilterQuery]);

return null;
}
Expand Down
Loading