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

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

Merged
merged 12 commits into from
Aug 27, 2020

Conversation

Zacqary
Copy link
Contributor

@Zacqary Zacqary commented Aug 19, 2020

Summary

Closes #70728

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.

@Zacqary 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 Zacqary requested a review from a team as a code owner August 19, 2020 22:30
@elasticmachine
Copy link
Contributor

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

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

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. 😇

Comment on lines 86 to 91
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]);
Copy link
Member

Choose a reason for hiding this comment

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

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

?

Comment on lines 15 to 18
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) => {
Copy link
Member

Choose a reason for hiding this comment

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

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) => {

@Zacqary
Copy link
Contributor Author

Zacqary commented Aug 24, 2020

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

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

Thanks for investing the time!

…shold/evaluate_condition.ts

Co-authored-by: Felix Stürmer <[email protected]>
@Zacqary
Copy link
Contributor Author

Zacqary commented Aug 26, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

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

History

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

@Zacqary Zacqary merged commit b98e2e4 into elastic:master Aug 27, 2020
@Zacqary Zacqary deleted the 70728-typecheck-remove-any branch August 27, 2020 15:06
Zacqary added a commit to Zacqary/kibana that referenced this pull request Aug 27, 2020
)

Co-authored-by: Felix Stürmer <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
@kibanamachine
Copy link
Contributor

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 kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 31, 2020
@kibanamachine
Copy link
Contributor

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 kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 1, 2020
Zacqary added a commit that referenced this pull request Sep 1, 2020
…76108)

Co-authored-by: Felix Stürmer <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Felix Stürmer <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Metrics UI Metrics UI feature release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Metrics UI] Replace the uses of any introduced during the lodash 4 upgrade with proper typings
4 participants