Move interfaces for interpreter types to OSS #37753
Conversation
| type: typeof name; | ||
| spec: any; | ||
| css: string; | ||
| } |
There was a problem hiding this comment.
I added this interface because it didn't exist yet. Is this correct?
There was a problem hiding this comment.
Feel free to move these up.
There was a problem hiding this comment.
looks like it's getting moved up in #37619 anyway -- that PR is likely to merge sooner, so i'll just wait for it to land and rebase this PR.
lukeelmers
added
:AppArch
Team:Presentation
|
Pinging @elastic/kibana-canvas |
|
Pinging @elastic/kibana-app-arch |
💚 Build Succeeded |
There was a problem hiding this comment.
My comments... @w33ble, your input on intention would be helpful here, as well.
| paginate: true, | ||
| perPage: 10, | ||
| showHeader: true, | ||
| }, | ||
| }; | ||
| }, | ||
| pointseries: datatable => { | ||
| pointseries: (dtable): PointSeries => { |
There was a problem hiding this comment.
nit: is there a reason dtable is not typed inline? I'm sure it's coming from somewhere, but the fact I can't find it easily might mean an inline type that matches would be helpful.
There was a problem hiding this comment.
@clintandrewhall Yep, it's because this is already handled in the ExpressionType generic. We know that any function on a key in to is always going to cast the current type (which is passed into the generic) to something else. And we know that any function on a key in from is always going to return the current type (which is passed into the generic).
So TS should enforce here that the first arg to any of these to functions matches the Datatable interface
There was a problem hiding this comment.
But to your point, if that feels too "magical" it wouldn't hurt to explicitly say dtable: Datatable, it just isn't necessary as the generic already wires it up for you
|
|
||
| const name = 'image'; | ||
|
|
||
| export const image = (): ExpressionType<typeof name, undefined> => ({ |
There was a problem hiding this comment.
nit: do you mean undefined or do you mean any? Because undefined would imply that no one should have any context at all passed in, whereas any would mean the context is inconsequential... I think this more style than anything, though.
There was a problem hiding this comment.
I wasn't thinking about style here so much as what would happen if we were to go back and add a proper Image interface in the future... If we keep this as undefined, then TS will blow up as soon as you start writing other methods that rely on this being anything other than undefined.
Whereas if we put any, it would be easier to forget to come back and put the proper parameter in place later, giving us a false sense of type safety.
There was a problem hiding this comment.
Actually, I'm realizing now this doesn't need to be undefined at all, since we have a type for Image already in the Canvas image() function. Will update that here too.
| /** | ||
| * Allowed column names in a PointSeries | ||
| */ | ||
| export type PointSeriesColumnName = 'x' | 'y' | 'color' | 'size' | 'text'; |
There was a problem hiding this comment.
I would convert this to an enum.
There was a problem hiding this comment.
Copied this straight from Canvas but I agree an enum might be useful here.
I'm going to leave as-is for now because I don't want the implementations between Canvas & this PR to diverge (since I'm not deleting any Canvas code here). To keep PRs clean, I'd prefer to wait until Canvas has flipped over to using all of these types, and then change it over at that point.
| type: typeof name; | ||
| spec: any; | ||
| css: string; | ||
| } |
There was a problem hiding this comment.
Feel free to move these up.
lukeelmers
force-pushed
the
feat/interpreter-types-oss
branch
from
817399e to
52b752c
Compare
💚 Build Succeeded |
| * A generic type which represents a custom Expression Type Definition that's | ||
| * registered to the Interpreter. | ||
| */ | ||
| export interface ExpressionTypeDef<Name extends string, Type, SerializedType = undefined> { |
There was a problem hiding this comment.
nit: this is personal style, but I got confused by this name. Seeing it in other files had me thinking it meant type as in Typescript and def was redundant. Naming is the hardest thing, so it's up to you.
There was a problem hiding this comment.
Ah this is a tough one; I thought the same thing about potential for confusion when using "def". I had changed it because @w33ble pointed out that elsewhere in Canvas, we refer to these as the Type "Definitions"; I'm happy to revert though, as I don't feel strongly one way or the other.
There was a problem hiding this comment.
I'm torn, honestly. Some other options that might be less confusing:
- Expression (isn't that really what this is?)
- ExpressionTypeDefinition
- ExpressionTD (ugh)
There was a problem hiding this comment.
Expression (isn't that really what this is?)
No, this is a definition for the various data types that the expression handles. And there's a distinction between a type's definition (the object this interface is defining) and it's instance (what is provided by the TypeRegistry).
I mentioned preferring adding something extra to the ExpressionType name, since there will also be an instance of ExpressionType that is created using this definition.
💚 Build Succeeded |
lukeelmers
added
release_note:skip
Currently Canvas maintains interfaces for the various types used in the Interpreter. This PR does the following:
ExpressionFunctiongeneric which was added in Expression typings: Canvas to OSS #37438 expects users to provide Context and Return types, so this way users will be able to import some of the commonly used types directly from the interpreter and provide those toExpressionFunction).ExpressionTypeDefgeneric that defines the interface for interpreter type specs.I have specifically avoided removing the duplicate types from Canvas in this PR to keep things small. In a subsequent PR we can remove those duplicates and update the Canvas imports so everything is sharing the same interfaces.
Note to reviewers: There was some weirdness when Github diffed these changes... It shows completely separate files as being moved & I have no idea why. It's probably easiest to review this commit-by-commit instead of looking at the overall diff.