[InfraUI] Remove (to be deprecated) rest_total_hits_as_int parameter #30826
Conversation
Change checkValidNode response parsing Update type
Kerry350
added
review
Feature:Metrics UI
|
Pinging @elastic/infrastructure-ui |
💚 Build Succeeded |
There was a problem hiding this comment.
.____ ________________________
| | / _____/\__ ___/ \
| | / \ ___ | | / \ / \
| |__\ \_\ \ | |/ Y \
|_______ \______ / |____|\____|__ /
\/ \/ \/
I added one note but it's not required to merge. You can merge without it but I wanted to get your thoughts on the idea.
| return (await search(params)).hits.total > 0; | ||
|
|
||
| const result = await search(params); | ||
| return result && result.hits && result.hits.total && result.hits.total.value > 0; |
There was a problem hiding this comment.
I've been struggling with things like this too where we need to grab a value off some deeply nested object that has optional parts all the way down 🐢. I wonder if we shouldn't just standardize on using Lodash get for things like this since we are only concerned with the value at the end of the path. That's was standard practice at one point in time for the Kibana team. I only started using the same pattern as you when I started with Typescript.
const result = await search(params)
const value = get(result, 'hits.total.value', 0);
return value > 0;You don't need to make this change, unless you like it. Technically they are both the same so I will let you decide.
There was a problem hiding this comment.
The APM team has implemented some typesafer getter pattern which we might want to copy: https://github.com/elastic/kibana/blob/master/x-pack/plugins/apm/common/idx.ts
example usage here:
There was a problem hiding this comment.
We should settle on something because personally I don't like typing result & result.total & result.total.value, I like editing it even less, especially when the structure changes.
There was a problem hiding this comment.
indeed... the idx (which could be named better) has a few nice properties:
- preserves the return type
- auto-completion in the accessor
- simple, local implementation
There was a problem hiding this comment.
Yep, I agree it's annoying and if we can decide on a standardised solution (be it Lodash's get, APM's idx, or a tweaked custom function of our own) that'd be great. I'll merge this as is for now as it's managed to get a green build amid the CI issues. But I'll add an agenda issue for the next Infrastructure & Service Monitoring UI Sync meeting and we can decide which to use.
…35792) * [Infra UI] Add new graphql endpoint for snapshot data (#34264) * Add new graphql endpoint for snapshot data * Polishing. * Keep type generic that is used outside snapshots * Keep one more generic type generic * Use camelCase for consistency. * Refine type names * Add return types. * Use idiomatic javascript. * Factor out getAllCompositeAggregationData<T>() * Refine naming. * More idiomatic JavaScript, more types. * Use pre-8.x response format (see also #30826)
timroes
added
the
release_note:skip
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
kibanamachine
added
the
backport missing
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
kibanamachine
removed
the
backport missing
Summary
Addresses: #29435
Overview: Removes our use of the to be removed Elasticsearch
rest_total_hits_as_int=trueparameter.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] 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