-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Typescript es-query functions #57473
Typescript es-query functions #57473
Conversation
Pinging @elastic/kibana-app-arch (Team:AppArch) |
@elasticmachine merge upstream |
…izozom/kibana into newplatform/data/ts-es-query-functions
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Types look reasonable to me from just reading the code. I wasn't able to bootstrap though due to a type error in function.ts
. Otherwise LGTM. Definitely have @lukasolson take a look at this too before merging, I just transferred all my most recent KQL knowledge to him so he'll be the best reviewer for these sorts of changes going forward.
…izozom/kibana into newplatform/data/ts-es-query-functions
…ata/ts-es-query-functions
…ata/ts-es-query-functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some notes below, just a few minor changes requested and nits.
Just wanted to document a few improvements we could continue to make after this is in:
- Create types for each of the different "function" node types (e.g.
AndNode
,OrNode
, etc.) - Create type for
config
(I think it contains things likeallowLeadingWildcards
and potentially other UI settings) - Create type for
context
(which I think only contains things about whether it'snested
or not, but this would take some research) - Add types for the
params
given to geoBoundingBox/geoPolygon/nested - Parameterize the
literal.buildNode
(so you can do something likeliteral.buildNode<string>
) - Potentially add types for the results of
toElasticsearchQuery
(although I think these would probably belong in ourelasticsearch
dependency) - Add types for the
fieldName
arg (that could be a node/string I believe) that is passed in several places - Get rid of the
as LiteralTypeBuildNode
by addingis
functions that are exported from types - Audit where we've added
@ts-ignore
statements and build out the parameterized types to get rid of them
src/plugins/data/common/es_query/kuery/functions/geo_bounding_box.ts
Outdated
Show resolved
Hide resolved
src/plugins/data/common/es_query/kuery/functions/geo_polygon.ts
Outdated
Show resolved
Hide resolved
src/plugins/data/common/es_query/kuery/functions/geo_polygon.ts
Outdated
Show resolved
Hide resolved
…ata/ts-es-query-functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* TS Functions * Rename index.ts * tsing * JSON Value * ts ignore * TS adjustments * context?.nested?.path * Code review * Import LiteralTypeBuildNode * Fix jest tests * revert test * TS fix IFieldType * Remvoe unnecessary casting * range types Co-authored-by: Elastic Machine <[email protected]>
* TS Functions * Rename index.ts * tsing * JSON Value * ts ignore * TS adjustments * context?.nested?.path * Code review * Import LiteralTypeBuildNode * Fix jest tests * revert test * TS fix IFieldType * Remvoe unnecessary casting * range types Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
Summary
Part of #56881
Basic Typescript for es-query functions
Checklist
Delete any items that are not applicable to this PR.
For maintainers