[expressions] AST Builder#64395
Conversation
aaee2e7 to
9a15484
Compare
| export function formatExpression(ast: ExpressionAstExpression): string { | ||
| return format(ast, 'expression'); | ||
| } |
There was a problem hiding this comment.
split this into a separate file for consistency with how we split parse_expression into a separate file
| export function parse<E extends string, S extends 'expression' | 'argument'>( | ||
| expression: E, | ||
| startRule: S | ||
| ): S extends 'expression' ? ExpressionAstExpression : ExpressionAstArgument { |
There was a problem hiding this comment.
I ran into some issues with the typings for parse and format which were requiring unnecessary casting -- so I updated these types to use conditionals which is more accurate.
|
|
||
| // Fallback for if a function cannot be found in the mapping, | ||
| // or someone forgets to `declare module`. | ||
| [key: string]: AnyExpressionFunctionDefinition; |
There was a problem hiding this comment.
Adding this felt like the safest thing to do, however AnyExpressionFunctionDefinition is extremely permissive -- it basically uses any for everything. This could probably be made more strict.
|
@elasticmachine merge upstream |
|
This looks really nice. I know there are a lot of places in Canvas where we manually traverse through an expression to update it (I think maybe all of the "sidebar" stuff that updates the expression does that). I also do a lot of manual building of expressions when dealing with an embeddable's input changing through user interaction. (Like when a user pans a map, the expression needs to be updated to reflect the new center coordinates) and it would be much nicer to build it using this that through the string manipulation I currently do. Off the top of my head, I can't think of any use cases we might have for it that this wouldn't cover. Really nice work. Anxious to try it out. |
|
@elasticmachine merge upstream |
| /** | ||
| * To support subexpressions, we override all args to also accept an | ||
| * ExpressionBuilder. This isn't perfectly typesafe since we don't | ||
| * know with certainty that the builder's output matches the required | ||
| * argument input, so we trust that folks using subexpressions in the | ||
| * builder know what they're doing. | ||
| */ | ||
| initialArgs: { [K in keyof FunctionArgs<F>]: FunctionArgs<F>[K] | ExpressionAstExpressionBuilder } |
There was a problem hiding this comment.
Highlighting this, as it's probably the main type-safety caveat:
The return value of buildExpression is added as an option for any expression function argument, so if someone is creating a subexpression they must take care to ensure that the expected return value of that whole expression actually satisfies the requirements of the argument it is passed to.
This is unfortunately not something we can determine for them in TS, as the output of an expression is equivalent to the output of the last function in that expression, which is something we don't know at compile time.
| font?: Style; | ||
| openLinksInNewTab?: boolean; |
There was a problem hiding this comment.
These should be optional as default values are provided in the function definition
|
@elasticmachine merge upstream |
e510ee6 to
8a691ec
Compare
|
@elasticmachine merge upstream |
1 similar comment
|
@elasticmachine merge upstream |
|
As discussed offline, the |
Thanks @wylieconlon -- I've updated this based on our discussion. Any subexpression arguments to a function are now wrapped in the builder, rather than as raw AST. That means calling |
|
Okay @lukeelmers I've tested out writing a migration with this new code: as you've pointed out, we need to extend the shared types for expression functions to include the functions we are using such as So overall, I think this API is very close to being ready. The thing that would make me feel most comfortable with it is seeing how you would use it to write a real migration, for two reasons:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@elasticmachine merge upstream |
|
Discussed this PR when doing some planning for the expressions service today, and @timroes & I agreed to revert the changes to Lens, which were only added to prove that this builder works. We felt that touching migrations which have shipped already introduces unnecessary risk to a PR which is otherwise simply adding a new feature. |
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
History
To update your PR or re-run it, just comment with: |
|
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. |
|
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. |
Closes #56748
Prior POCs & discussions: #62109, #62334, #34318
cc @elastic/kibana-canvas, @elastic/kibana-app, @elastic/kibana-app-arch
Summary
Adds two utilities for managing Expression ASTs:
buildExpressionandbuildExpressionFunction. They provide a type-safe mechanism for manipulating & migrating expressions without doing the work of manually traversing the AST.If you work on an app that uses Expressions, and you find yourself updating an Expression AST directly or -- gasp 😱 -- concatenating Expression strings -- this will make your life easier.
Usage
There's also a
buildExpression.findFunctionmethod which aims to make migrations easier by recursively searching for all functions in an expression matching a given name (including subexpressions). It returns references to thebuildExpressionFunctions for each of the found functions, which allows you to iterate over each of them to do your migration. There's a unit test showing an example of this, but basically:Checklist
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportThis was checked for keyboard-only and screenreader accessibilityThis renders correctly on smaller devices using a responsive layout. (You can test this in your browserThis was checked for cross-browser compatibility, including a check against IE11Dev Docs
The
expressionsplugin has introduced a set of helpers which make it easier to manipulate expression ASTs. Please refer to the PR for more detailed examples.