TS of esKuery\node_types #56857
TS of esKuery\node_types #56857
Conversation
|
Pinging @elastic/kibana-app-arch (Team:AppArch) |
lizozom
added
the
release_note:skip
lizozom
requested review from
lukeelmers
and removed request for
a team and
Bamieh
| type: 'literal'; | ||
| value: null | boolean | number | string; | ||
| } | ||
|
|
||
| interface NamedArgType { | ||
| buildNode: (name: string, value: any) => NamedArgTypeBuildNode; | ||
| toElasticsearchQuery: (node: any) => string; | ||
| toElasticsearchQuery: (node: any) => JsonValue; |
There was a problem hiding this comment.
For each of the toElasticsearchQuery() items in this file, should the node param they are accepting match the returned value of the buildNode() function?
For example here, I would expect the interface to be:
interface NamedArgType {
buildNode: (name: string, value: any) => NamedArgTypeBuildNode;
toElasticsearchQuery: (node: NamedArgTypeBuildNode) => JsonValue;
}...and similar for each of the others as well as the individual node_types definitions themselves.
I don't know enough about the kuery implementation to say with 100% confidence that this should be the case, but it makes sense based on my understanding of the code and would make the typings in this PR much simpler
There was a problem hiding this comment.
I agree with you, but I was trying to stick with the API defined in types.ts as much as possible, to avoid errors.
💚 Build SucceededTo update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Code changes LGTM. I do think we should revisit the toElasticsearchQuery typings as discussed, but there are plenty of other TS areas in esKuery that need attention, so perhaps that could be done as part of a wider refactor to tighten things up in a future PR.
* TS of node types and collector * Code review
…b.com:jloleysens/kibana into console/feature/text-objects-in-saved-objects * 'console/feature/text-objects-in-saved-objects' of github.com:jloleysens/kibana: (103 commits) fix auto closing new vis modal when navigating to lens or when navigating away with browser history (elastic#56998) TS of esKuery\node_types (elastic#56857) Kibana app migration: Move static code dependencies into kibana_legacy plugin, part 1 (elastic#56408) Retry ES API calls that fail with 410/Gone (elastic#56950) [APM] Show missing permissions message to the user on the Services overview (elastic#56374) Fixing flaky CI tests for custom appRoutes (elastic#55763) [State Management][Docs] State syncing utils docs (elastic#56479) [Index management] Remove index mapper setting in tests (elastic#57066) Exposed common EuiExpressions to separate components be able to reuse for building new for Alert Types (elastic#56466) [SIEM] update url state between page if date is relative (elastic#56813) fix for chart_types test (elastic#57056) chore(NA): remove compress from dll minimizer (elastic#57023) [File upload] Migrate routing to NP & add route validation (elastic#52313) Adding docs for grouped nav advanced setting (elastic#57013) Use i18n titles for field formatters, human names for numeral locales (elastic#56348) [Maps] Remove EMS catalogue url from docs (elastic#57020) [Endpoint] ERT-82 ERT-83 ERT-84: Alert list API with pagination (elastic#56538) [DOCS] Adds Apple notarization info to install doc (elastic#57042) [ML] New Platform server shim: update results service routes to use new platform router (elastic#56886) Fix typo on detection engine rule (elastic#56993) ...
Summary
Typescript-ification of esKuery node_types
Part of #56881
Checklist
Delete any items that are not applicable to this PR.
For maintainers