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 all 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 autoFocusInputElement = useCallback(
(inputElement: HTMLInputElement | null) => {
if (inputElement && shouldFocus) {
inputElement.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 All @@ -86,7 +81,7 @@ export const MetricsExplorerMetrics = ({ options, onChange, fields, autoFocus =
selectedOptions={selectedOptions}
onChange={handleChange}
isClearable={true}
inputRef={handleInputRef}
inputRef={autoFocusInputElement}
/>
);
};
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 @@ -125,23 +125,23 @@ export const useLogAnalysisModule = <JobType extends string>({
dispatchModuleStatus({ type: 'failedSetup' });
});
},
[cleanUpModule, setUpModule]
[cleanUpModule, dispatchModuleStatus, setUpModule]
);

const viewSetupForReconfiguration = useCallback(() => {
dispatchModuleStatus({ type: 'requestedJobConfigurationUpdate' });
}, []);
}, [dispatchModuleStatus]);

const viewSetupForUpdate = useCallback(() => {
dispatchModuleStatus({ type: 'requestedJobDefinitionUpdate' });
}, []);
}, [dispatchModuleStatus]);

const viewResults = useCallback(() => {
dispatchModuleStatus({ type: 'viewedResults' });
}, []);
}, [dispatchModuleStatus]);

const jobIds = useMemo(() => moduleDescriptor.getJobIds(spaceId, sourceId), [
moduleDescriptor.getJobIds,
moduleDescriptor,
spaceId,
sourceId,
]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ export const useAnalysisSetupState = <JobType extends string>({
? [...errors, ...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 @@ -25,11 +25,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 @@ -41,7 +41,7 @@ export const LogHighlightsFilterQueryBridge = withLogFilter(

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

return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export const WithStreamItems: React.FunctionComponent<{
createLogEntryStreamItem(logEntry, logEntryHighlightsById[logEntry.gid] || [])
),

[logEntries.entries, logEntryHighlightsById]
[isAutoReloading, logEntries.entries, logEntries.isReloading, logEntryHighlightsById]
);

return children({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ export function useMetricsExplorerData(
}
setLoading(false);
})();

// TODO: fix this dependency list while preserving the semantics
Copy link
Member Author

Choose a reason for hiding this comment

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

@simianhacker @phillipb I couldn't reliably decompose this effect to derive a correct dependency list. I'd appreciate your help here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@weltenwort Yeah, this is pretty gnarly. Does useEffect really need to have all of it's deps specified? I can think of use cases where you might want to use some data in an effect, but you don't necessarily want to rerun that effect every time the data changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming for now that it does, I'll need to spend a bit of time understanding this more and refactoring it. Are you ok, with me taking that on as a separate PR/issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely, let's take it to a different PR. I was planning to share some strategies for dealing with it in a session some time soon. In the meantime https://overreacted.io/a-complete-guide-to-useeffect/ is a nice deep dive into the topic.

// eslint-disable-next-line react-hooks/exhaustive-deps
}, [options, source, timerange, signal, afterKey]);
return { error, loading, data };
}
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ function useStateWithLocalStorage<State>(
const [state, setState] = useState<State>(parseJsonOrDefault<State>(storageState, defaultState));
useEffect(() => {
localStorage.setItem(key, JSON.stringify(state));
}, [state]);
}, [key, state]);
return [state, setState];
}

Expand Down
Loading