[Metrics UI] Replace uses of `any` introduced by Lodash 4 #75507

Zacqary · 2020-08-19T22:30:24Z

Summary

Replaces the any castings introduced by the Lodash 4 upgrade. A lot of adding ! to the end of first() and last() calls, because Typescript doesn't realize that if (someArray.length > 0) means that first/last called on someArray won't return undefined. But there were a few cases of just needing to clarify some of the typings.

elasticmachine · 2020-08-19T22:30:26Z

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

weltenwort

👀 Reviewing on behalf of @elastic/logs-metrics-ui...

weltenwort

Thanks for tackling this! 👍 I left some suggestions about avoiding assertions by reformulating in a readable manner. ✍️

In other places I questioned the safety of the assertions, because if the intention of the any removal is to improve type safety and the reduction of runtime errors, then simply replacing as any with ! in many places doesn't achieve that.

If that's not the goal of this PR then please feel free to ignore my nitpicky questions. 😇

weltenwort · 2020-08-20T09:54:17Z

x-pack/plugins/infra/public/alerting/metric_threshold/components/expression_chart.tsx

  const dateFormatter = useMemo(() => {
    const firstSeries = data ? first(data.series) : null;
    return firstSeries && firstSeries.rows.length > 0
-      ? niceTimeFormatter([
-          (first(firstSeries.rows) as any).timestamp,
-          (last(firstSeries.rows) as any).timestamp,
-        ])
+      ? niceTimeFormatter([first(firstSeries.rows)!.timestamp, last(firstSeries.rows)!.timestamp])
      : (value: number) => `${value}`;
  }, [data]);


This is an interesting one - even though it's semantically correct, first's and last's type signatures don't allow the compiler to infer it correctly. We could, however, try to reformulate it to something that is both readable and fully type-checked by relying on the ?. operator and the fact that first and last can handle undefined values. What do you think about

const dateFormatter = useMemo(() => { const firstSeries = first(data?.series); const firstTimestamp = first(firstSeries?.rows)?.timestamp; const lastTimestamp = last(firstSeries?.rows)?.timestamp; if (firstTimestamp == null || lastTimestamp == null) { return (value: number) => `${value}`; } return niceTimeFormatter([firstTimestamp, lastTimestamp]); }, [data?.series]);

?

weltenwort · 2020-08-20T10:02:33Z

x-pack/plugins/infra/public/alerting/metric_threshold/lib/transform_metrics_explorer_data.ts

  const { criteria } = params;
  if (criteria && data) {
-    const firstSeries = first(data.series) as any;
-    const series = firstSeries.rows.reduce((acc: any, row: any) => {
+    const firstSeries = first(data.series)!;
+    const series = firstSeries.rows.reduce((acc, row) => {


Isn't the assertion first(data.series)! risky without checking data.series.length? How about we bind the series to a name early and check it for undefined directly?

const { criteria } = params; const firstSeries = first(data?.series); if (criteria && firstSeries) { const series = firstSeries.rows.reduce((acc, row) => {

x-pack/plugins/infra/public/pages/metrics/inventory_view/lib/color_from_value.ts

x-pack/plugins/infra/public/pages/metrics/inventory_view/lib/nodes_to_wafflemap.ts

...a/server/lib/alerting/inventory_metric_threshold/preview_inventory_metric_threshold_alert.ts

x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.ts

x-pack/plugins/infra/server/lib/alerting/metric_threshold/preview_metric_threshold_alert.ts

x-pack/plugins/infra/server/lib/snapshot/response_helpers.ts

Co-authored-by: Felix Stürmer <weltenwort@users.noreply.github.com>

…a into 70728-typecheck-remove-any

…emove-any # Conflicts: # x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.ts

Zacqary · 2020-08-24T16:32:39Z

I think applying some suggestions locally and some through the GitHub UI confused GitHub and now it's not showing the up-to-date code changes. Going to force push to fix

…emove-any

weltenwort

Thanks for investing the time!

x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/evaluate_condition.ts

…shold/evaluate_condition.ts Co-authored-by: Felix Stürmer <weltenwort@users.noreply.github.com>

Zacqary · 2020-08-26T18:23:14Z

@elasticmachine merge upstream

kibanamachine · 2020-08-26T20:12:36Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 15982ea

Build metrics

async chunks size

id	value	diff	baseline
infra	3.6MB	+562.0B	3.6MB

History

💚 Build #70324 succeeded a4c331f
💚 Build #70063 succeeded 834ffb0
💛 Build #69742 was flaky 35daafa
💔 Build #69734 failed 68e2b0b
💚 Build #69516 succeeded 3940c8e

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

) Co-authored-by: Felix Stürmer <weltenwort@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

kibanamachine · 2020-08-31T15:08:44Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine · 2020-09-01T15:18:44Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

…76108) Co-authored-by: Felix Stürmer <weltenwort@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Felix Stürmer <weltenwort@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

[Metrics UI] Replace uses of any introduced by Lodash 4

63d3061

Zacqary added Feature:Metrics UI Metrics UI feature v8.0.0 Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Aug 19, 2020

Zacqary requested a review from a team as a code owner August 19, 2020 22:30

weltenwort reviewed Aug 20, 2020

View reviewed changes

Zacqary and others added 8 commits August 20, 2020 11:37

Address PR feedback

0d67d30

Apply suggestions from code review

8eb66ab

Co-authored-by: Felix Stürmer <weltenwort@users.noreply.github.com>

Fix typecheck

831da52

Merge branch '70728-typecheck-remove-any' of github.com:Zacqary/kiban…

553daee

…a into 70728-typecheck-remove-any

Merge remote-tracking branch 'upstream/master' into 70728-typecheck-r…

4f6652e

…emove-any # Conflicts: # x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.ts

Fix typecheck

3940c8e

Improve type safety of n.path

68e2b0b

Fix type conflicts

35daafa

Zacqary mentioned this pull request Aug 21, 2020

[Metrics UI] Improve type safety of SnapshotNodePath #75674

Closed

Merge remote-tracking branch 'upstream/master' into 70728-typecheck-r…

834ffb0

…emove-any

weltenwort reviewed Aug 25, 2020

View reviewed changes

x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/evaluate_condition.ts Outdated Show resolved Hide resolved

Update x-pack/plugins/infra/server/lib/alerting/inventory_metric_thre…

a4c331f

…shold/evaluate_condition.ts Co-authored-by: Felix Stürmer <weltenwort@users.noreply.github.com>

Merge branch 'master' into 70728-typecheck-remove-any

15982ea

weltenwort approved these changes Aug 27, 2020

View reviewed changes

Zacqary merged commit b98e2e4 into elastic:master Aug 27, 2020

Zacqary deleted the 70728-typecheck-remove-any branch August 27, 2020 15:06

Zacqary mentioned this pull request Aug 27, 2020

[7.x] [Metrics UI] Replace uses of any introduced by Lodash 4 (#75507) #76108

Merged

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 31, 2020

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 1, 2020

[Metrics UI] Replace uses of any introduced by Lodash 4 #75507

[Metrics UI] Replace uses of any introduced by Lodash 4 #75507

Uh oh!

Conversation

Zacqary commented Aug 19, 2020

Summary

Uh oh!

elasticmachine commented Aug 19, 2020

Uh oh!

weltenwort left a comment

Choose a reason for hiding this comment

Uh oh!

weltenwort left a comment

Choose a reason for hiding this comment

Uh oh!

weltenwort Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

weltenwort Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Zacqary commented Aug 24, 2020

Uh oh!

weltenwort left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Zacqary commented Aug 26, 2020

Uh oh!

kibanamachine commented Aug 26, 2020

💚 Build Succeeded

Build metrics

async chunks size

History

Uh oh!

kibanamachine commented Aug 31, 2020

Uh oh!

kibanamachine commented Sep 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[Metrics UI] Replace uses of `any` introduced by Lodash 4 #75507

[Metrics UI] Replace uses of `any` introduced by Lodash 4 #75507