-
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
[expressions] AST Builder #64395
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
code LGTM
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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:
buildExpression
andbuildExpressionFunction
. 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.findFunction
method 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 thebuildExpressionFunction
s 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
expressions
plugin has introduced a set of helpers which make it easier to manipulate expression ASTs. Please refer to the PR for more detailed examples.