[Logs + Metrics UI] Remove eslint exceptions #50979

weltenwort · 2019-11-18T21:56:10Z

Summary

This removes the two eslint exceptions specific to the infra plugin introduced in #49244.

fixes #49563

Notes

The changes fall into one of several categories:

Those that fix incorrect dependency lists by adding or removing values. This class of errors was mostly auto-fixable.
Those that avoid the hook with the incorrect dependency list altogether by refactoring the code. This mostly applies to useEffects and might have changed the semantics.
Those that fix violations of the hook call order invariant by moving the condition either to a parent component or beneath the hook calls.
Those that selectively disable the linter check because the situation can not be statically checked.
Those that temporarily disable the linter check until the corresponding hook has been refactored to correctly model the dataflow.

Checklist

~~This was checked for cross-browser compatibility, including a check against IE11~~
~~Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~Documentation was added for features that require explanation or tutorials~~
~~Unit or functional tests were updated or added to match the most common scenarios~~
~~This was checked for keyboard-only and screenreader accessibility~~

elasticmachine · 2019-11-18T21:56:13Z

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

elasticmachine · 2019-11-18T23:13:54Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 7daf5b46636952c8fa9fae753e47fe7b975e0de4

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.

elasticmachine · 2019-12-03T01:09:53Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: c366a36

weltenwort · 2019-12-03T12:40:35Z

.../legacy/plugins/infra/public/containers/logs/log_analysis/log_analysis_results_url_state.tsx

@@ -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,


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

weltenwort · 2019-12-03T13:16:30Z

x-pack/legacy/plugins/infra/public/pages/metrics/components/sub_section.tsx

@@ -23,29 +23,25 @@ export const SubSection: FunctionComponent<SubSectionProps> = ({
  isLiveStreaming,
  stopLiveStreaming,
 }) => {
-  if (!children || !metrics) {


ℹ️ This condition violated the hook call order invariant so it was moved further down below the hook calls.

weltenwort · 2019-12-03T13:16:49Z

x-pack/legacy/plugins/infra/public/pages/metrics/components/sub_section.tsx

-        }
-        return null;
-      }),
-    [children, metric, id, onChangeRangeTime, isLiveStreaming, stopLiveStreaming]


ℹ️ Memoizing on children had no effect in this case because the parent always supplies a new list of React elements.

weltenwort · 2019-12-03T13:18:59Z

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 });


ℹ️ This always produced a new Moment and thereby caused the memoization below to have no effect. It was therefore moved into the memoization function's body so the original value could be used as the memoization key.

weltenwort · 2019-12-03T13:20:28Z

x-pack/legacy/plugins/infra/public/components/metrics_explorer/metrics.tsx

-    }
-  }, [inputRef]);
+  // the EuiCombobox forwards the ref to an input element
+  const handleInputRef = useCallback(


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

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

elasticmachine · 2019-12-03T14:21:28Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 8f7f34b

weltenwort · 2019-12-03T14:58:59Z

I would appreciate a look at the metric explorer fixes by someone more familiar with the intended semantics. Some of the required fixes were a bit more extensive, especially those were the calls were conditionally called.

weltenwort · 2019-12-09T15:36:23Z

x-pack/legacy/plugins/infra/public/pages/metrics/components/node_details_page.tsx

  addNavItem(item: NavItem): void;
  setRefreshInterval(refreshInterval: number): void;
  setAutoReload(isAutoReloading: boolean): void;
  triggerRefresh(): void;
  setTimeRange(timeRange: MetricsTimeInput): void;
 }
 export const NodeDetailsPage = (props: Props) => {
-  if (!props.metadata) {


ℹ️ This condition broke the hook call order invariant so it was moved to the parent component.

weltenwort · 2019-12-09T16:36:30Z

x-pack/legacy/plugins/infra/public/pages/metrics/index.tsx

-                  triggerRefresh={triggerRefresh}
-                  setTimeRange={setTimeRange}
-                />
+                {metadata ? (


ℹ️ This was moved up from the NodeDetailsPage component to fix the conditional hook usage. There's probably a better way of handling this than not rendering anything, but that's what the component itself did (@simianhacker @phillipb).

afgomez

Logs changes LGTM

afgomez · 2019-12-10T08:43:19Z

x-pack/legacy/plugins/infra/public/components/metrics_explorer/metrics.tsx

-    }
-  }, [inputRef]);
+  // the EuiCombobox forwards the ref to an input element
+  const handleInputRef = useCallback(


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

phillipb

This all looks really good. Not totally sure how I feel about useEffect needing to specify all of its deps, but it appears there's already a rich thread of folks talking through this facebook/create-react-app#6880. It'll probably just take me a bit to wrap my head around it.

phillipb · 2019-12-10T18:40:08Z

x-pack/legacy/plugins/infra/public/pages/metrics/components/chart_section_vis.tsx

-  if (!metric) {
+  if (!id) {
+    return null;
+  } else if (!metric) {


🤦‍♂looks right to me

elasticmachine · 2019-12-11T12:29:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c4dd7d4

History

💚 Build #14576 succeeded 1efb711
💚 Build #14456 succeeded 8e4fa68
💚 Build #14254 succeeded 13b7e90

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…le-sql-highlighting * 'master' of github.com:elastic/kibana: (56 commits) Migrate url shortener service (elastic#50896) Re-enable datemath in from/to canvas timelion args (elastic#52159) [Logs + Metrics UI] Remove eslint exceptions (elastic#50979) [Logs + Metrics UI] Add missing headers in Logs & metrics (elastic#52405) [ML] API integration tests - initial tests for bucket span estimator (elastic#52636) [Watcher] New Platform (NP) Migration (elastic#50908) Decouple Authorization subsystem from Legacy API. (elastic#52638) [APM] Fix some warnings logged in APM tests (elastic#52487) [ui/public/utils] Delete unused base_object & find_by_param (elastic#52500) [ui/public/utils] Move items into ui/vis (elastic#52615) fix newlines in kbn-analytics build script Add top level examples folder and command to run, `--run-examples`. (elastic#52027) feat(NA): add trap for SIGINT in the git precommit hook (elastic#52662) [DOCS] Updtes description of elasticsearch.requestHeadersWhitelist (elastic#52675) [Telemetry/Pulse] Updates advanced settings text for usage data (elastic#52657) [SIEM][Detection Engine] Adds the default name space to the end of the signals index [Logs UI] Generalize ML module management (elastic#50662) Removing stateful saved object finder (elastic#52166) Shim oss telemetry (elastic#51168) [Reporting/Screenshots] Do not fail the report if request is aborted (elastic#52344) ... # Conflicts: # src/legacy/core_plugins/console/public/legacy.ts # src/legacy/core_plugins/console/public/np_ready/application/models/legacy_core_editor/mode/elasticsearch_sql_highlight_rules.ts # src/legacy/core_plugins/console/public/np_ready/lib/autocomplete/components/full_request_component.ts # src/legacy/core_plugins/console/public/quarantined/src/sense_editor/row_parser.js

weltenwort · 2019-12-12T11:13:37Z

~~backport to 7.x blocked by #52582~~ (not anymore)

This removes the two eslint exceptions specific to the `infra` plugin introduced in elastic#49244. fixes elastic#49563

Backports the following commits to 7.x: - [Logs + Metrics UI] Remove eslint exceptions (#50979)

weltenwort self-assigned this Nov 18, 2019

weltenwort added 5 commits December 2, 2019 13:39

Remove infra eslint exception

214c285

Enhance useUrlState to write default to URL

9aa42ed

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.

Refactor autoFocus ref handler to avoid useEffect

6a660d4

Refactor onChange callback to avoid useEffect

4e0f0db

Fix incorrect dependency lists

c366a36

weltenwort force-pushed the logs-metrics-ui-remove-eslint-exceptions branch from 7daf5b4 to c366a36 Compare December 2, 2019 23:52

weltenwort added 12 commits December 3, 2019 12:01

Fix conditional hook usage and ineffective memoization

db84a98

Fix dangerous array mutation of useMemo closure

8a58208

Fix incorrect memoization dependencies

3ffbfdf

Whitelist externally managed dependency lists

fadb8ba

Fix incomplete dependency lists

c53a133

Fix conditional hook usage

873031a

Fix conditional hook usage

af5d428

Whitelist external dependencies

35a0902

Fix incomplete dependency lists

8a85124

Temporarily whitelist complex effect dependency list

5e523b6

Avoid useEffect for initialization

61d7095

Fix incomplete dependency lists

35bf164

weltenwort commented Dec 3, 2019

View reviewed changes

Wrap all imperative calls into useEffect

8f7f34b

weltenwort commented Dec 3, 2019

View reviewed changes

weltenwort marked this pull request as ready for review December 3, 2019 14:56

weltenwort requested a review from a team as a code owner December 3, 2019 14:56

weltenwort requested review from simianhacker and phillipb December 3, 2019 14:57

weltenwort added 2 commits December 9, 2019 16:32

Merge branch 'master' into logs-metrics-ui-remove-eslint-exceptions

24270c8

Fix conditional hook usage

13b7e90

weltenwort commented Dec 9, 2019

View reviewed changes

Merge branch 'master' into logs-metrics-ui-remove-eslint-exceptions

8e4fa68

afgomez self-requested a review December 10, 2019 08:42

afgomez approved these changes Dec 10, 2019

View reviewed changes

Name function more specifically

1efb711

phillipb approved these changes Dec 10, 2019

View reviewed changes

Merge branch 'master' into logs-metrics-ui-remove-eslint-exceptions

c4dd7d4

weltenwort merged commit 0cd62ca into elastic:master Dec 11, 2019

weltenwort deleted the logs-metrics-ui-remove-eslint-exceptions branch December 11, 2019 12:48

weltenwort mentioned this pull request Dec 12, 2019

[7.x] [Logs + Metrics UI] Remove eslint exceptions (#50979) #52933

Merged

weltenwort added a commit to weltenwort/kibana that referenced this pull request Dec 12, 2019

[Logs + Metrics UI] Remove eslint exceptions (elastic#50979)

6d3e729

This removes the two eslint exceptions specific to the `infra` plugin introduced in elastic#49244. fixes elastic#49563

weltenwort added a commit that referenced this pull request Dec 12, 2019

[7.x] [Logs + Metrics UI] Remove eslint exceptions (#50979) (#52933)

a0a749e

Backports the following commits to 7.x: - [Logs + Metrics UI] Remove eslint exceptions (#50979)

weltenwort mentioned this pull request Mar 10, 2020

[Logs UI] Use the Super date picker in the log stream #54280

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Logs + Metrics UI] Remove eslint exceptions #50979

[Logs + Metrics UI] Remove eslint exceptions #50979

weltenwort commented Nov 18, 2019 •

edited

Loading

elasticmachine commented Nov 18, 2019

elasticmachine commented Nov 18, 2019

elasticmachine commented Dec 3, 2019

weltenwort Dec 3, 2019

weltenwort Dec 3, 2019

weltenwort Dec 3, 2019

weltenwort Dec 3, 2019

weltenwort Dec 3, 2019

afgomez Dec 10, 2019

elasticmachine commented Dec 3, 2019

weltenwort commented Dec 3, 2019

weltenwort Dec 9, 2019

weltenwort Dec 9, 2019

afgomez left a comment

afgomez Dec 10, 2019

phillipb left a comment

phillipb Dec 10, 2019

elasticmachine commented Dec 11, 2019

weltenwort commented Dec 12, 2019 •

edited

Loading

[Logs + Metrics UI] Remove eslint exceptions #50979

[Logs + Metrics UI] Remove eslint exceptions #50979

Conversation

weltenwort commented Nov 18, 2019 • edited Loading

Summary

Notes

Checklist

elasticmachine commented Nov 18, 2019

elasticmachine commented Nov 18, 2019

💔 Build Failed

elasticmachine commented Dec 3, 2019

💔 Build Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Dec 3, 2019

💚 Build Succeeded

weltenwort commented Dec 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afgomez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phillipb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Dec 11, 2019

💚 Build Succeeded

History

weltenwort commented Dec 12, 2019 • edited Loading

weltenwort commented Nov 18, 2019 •

edited

Loading

weltenwort commented Dec 12, 2019 •

edited

Loading