[Canvas] Add Types to Functions by clintandrewhall · Pull Request #35087 · elastic/kibana

clintandrewhall · 2019-04-15T16:18:43Z

Summary

~~Depends on #35749~~ (merged).

Addresses #40160

This PR isn't easy. This prepares for i18n by adding strong types to Canvas Functions. I'm publishing this as a WIP to gather feedback as I go.

It's taken me several days to get Typescript working with Canvas Functions. But, as a result of that effort, we have a ton of new features to support i18n:

Define and Enforce Function Arguments

Canvas Functions have their arguments defined as individual properties of the args property. Each argument has one or more types, regardless of if it's expecting an array or single value of that type, expressed as an array of strings:

args: {
    name: {
      types: ['string'],
    },
    names: {
      types: ['string'], // Same as above, but...
      multi: true,       // ...the argument will be an array
    },
  },
},

This difference is now typed, and the type of each argument is available to fn:

In addition, the presence of the resolve property set to false will now enforce that the argument types be functions:

Define and Enforce Context

By providing a generic type to represent the context passed into the Function, we can enforce it in fn:

elasticmachine · 2019-04-15T16:18:47Z

Pinging @elastic/kibana-canvas

elasticmachine · 2019-04-15T16:59:08Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

lukasolson · 2019-04-15T18:45:38Z

Further, the types array is omitted if the argument has the resolve property set to false.

Not sure I'm understanding correctly, but does this mean things like ply will have errors?

kibana/x-pack/plugins/canvas/canvas_plugin_src/functions/common/ply.js

Lines 72 to 83 in f2388db

    
           expression: { 
        
             types: ['datatable'], 
        
             resolve: false, 
        
             multi: true, 
        
             aliases: ['fn', 'function'], 
        
             help: 
        
               'An expression to pass each resulting data table into. Tips: \n' + 
        
               ' Expressions must return a datatable. Use `as` to turn literals into datatables.\n' + 
        
               ' Multiple expressions must return the same number of rows.' + 
        
               ' If you need to return a differing row count, pipe into another instance of ply.\n' + 
        
               ' If multiple expressions return the same columns, the last one wins.', 
        
           },

resolve: false and types: [] are not supposed to be incompatible. When invoking the function to resolve the arguments, we check if the types match what the spec says. So, in something like ply where the expression argument is expected to return a datatable, if you don't actually return a datatable, we should throw an error.

clintandrewhall · 2019-04-15T21:28:22Z

@lukasolson Nice catch. @w33ble can you explain the relationship between resolve and types? Comparing when and then from case, for example?

elasticmachine · 2019-04-16T20:03:55Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

w33ble · 2019-04-17T19:37:31Z

Is there a reason you defined the type as FunctionFactory<Name, Arguments, Return, Context, Errors>?

Seems like it would fit better if you did context than arguments, since that's the order of the arguments in fn. ie. FunctionFactory<Name, Context, Arguments, Return, Errors>

clintandrewhall · 2019-04-17T19:39:59Z

@w33ble I can change the order, but you'd be forced to add void in a lot of function cases, like any, which don't have a context defined. I'm honestly easy with whatever order, I was just optimizing for terseness.

clintandrewhall · 2019-04-17T19:41:18Z

@w33ble For example:

export const any: FunctionFactory<'any', Arguments, boolean> = () => {
  const { help, args: argHelp } = getFunctionHelp().any;

would become

export const any: FunctionFactory<'any', void, Arguments, boolean> = () => {
  const { help, args: argHelp } = getFunctionHelp().any;

elasticmachine · 2019-04-17T20:20:54Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-18T16:00:57Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

w33ble · 2019-04-18T16:07:39Z

@clintandrewhall I think FunctionFactory<'any', void, Arguments, boolean> is fine, and it has the advantage of being explicit. Also, isn't there a way to define multiple possible shapes? So that both FunctionFactory<'any', void, Arguments, boolean> and FunctionFactory<'any', Arguments, boolean> are valid? Or maybe adding a related ContextlessFunctionFactory type makes sense for functions that don't use context? You're the ts guy, you'd know more than me, it just seems weird that the argument orders don't match.

clintandrewhall · 2019-04-22T16:32:46Z

As requested, I've updated this PR to only include the TS changes. I'm still making some changes around naming.

elasticmachine · 2019-04-22T16:43:47Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-22T18:25:46Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-22T19:08:01Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-26T00:40:52Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

monfera · 2019-04-26T18:16:48Z

+/**
+ * Utility type: gathers values of a collection as a type for use as a type.
+ */
+export type ValuesOf<T extends any[]> = T[number];


TS ramp-up (for me) question, what's the difference between this and eg. Array<T>?

You should only use Array<T> for complex type expressions, (and eslint will complain if you deviate). For example: string[] and FooType[] are preferable to Array<string> or Array<FooType>, but Array<{foo: string}> is preferable in that case.

I read it - correctly? - as:

"You should use Array for complex type expressions only" (ie. not for simple expressions, though I'm not yet clear on what constitutes simple vs complex)

string[] and FooType[] are semantically, but not stylistically equivalent to Array<string> and Array<FooType>, resp.

I tried to trigger the syntax check, entering

export type ValuesOf<T extends any[]> = Array<T>;

but it didn't balk, maybe it still accepts it or I don't run the right linter. Also, ValuesOf and UnionToIntersection seem to be unused atm. All these are only curiosities, not sure if they're important at all.

I think generics may be an exception...?

monfera · 2019-04-26T18:51:12Z

+ */
+// prettier-ignore
+export type AvailableFunctionNames = 
+  AvailableFunctions<typeof commonFunctions[number]>['name'] &


First of all, incredible work, learning so much just by looking through this! How can AvailableFunctionNames be used? It's not being referred to now, does it have an impact without use, or if not, what can in theory be hooked up to it?

This is going to be used for i18n, for starters. You'll see it's usefulness in my next diff.

Here's a practical example. In this case, the AvailableFunctions type can be used to enforce the structure of the FunctionHelpDict type, throwing a TS error if any of the functions are missing.

/** This type uses the collection of available functions, (which are all * `FunctionFactory`), to derive the name and argument types from them. This * allows for validation that 1/ every function available is listed, and 2/ every * function argument has a help string present. */ type FunctionHelp<T> = T extends FunctionFactory< infer Name, infer Context, infer Arguments > ? { [key in Name]: { help: string; args: { [key in keyof Arguments]: string }; } } : never; /** * This type represents an exhaustive dictionary of Function help strings, * organized by Function and then Function Argument. * * This type indexes the existing function factories, reverses the union to an * intersection, and produces the dictionary of strings. */ type FunctionHelpDict = UnionToIntersection<FunctionHelp<typeof functions[number]>>; /** * Help text for Canvas Functions should be properly localized. This function will * return a dictionary of help strings, organized by Canvas Function specification * and then by available arguments. */ export const getFunctionHelp = (): FunctionHelpDict => { return { alterColumn: { help: i18n.translate('xpack.canvas.functions.alterColumnHelpText', { defaultMessage: 'Converts between core types, eg string, number, null, boolean, date and rename columns', }), args: { column: i18n.translate('xpack.canvas.functions.alterColumn.args.columnHelpText', { defaultMessage: 'The name of the column to alter', }), type: i18n.translate('xpack.canvas.functions.alterColumn.args.typeHelpText', { defaultMessage: 'The type to convert the column to. Leave blank to not change type', }), name: i18n.translate('xpack.canvas.functions.alterColumn.args.nameHelpText', { defaultMessage: 'The resultant column name. Leave blank to not rename', }), }, },

elasticmachine · 2019-04-29T18:40:13Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-29T19:43:00Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

monfera

Incredible, tight fitting types PR!

monfera

Thanks @clintandrewhall, I learnt a ton from this PR and there's still much to absorb!

elasticmachine · 2019-05-02T16:50:33Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-02T18:29:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Reducing changeset to TS-only * Update names; fix a few bugs * Test fixes * Add browser and server functions * Update ts-ignore comments; convert pie; break up TS defs * Convert const to func * Plot conversion * Tweaks to documentation * Fix deleted file * Fix type errors * Update yarn.lock * Address feedback

elasticmachine · 2019-07-02T15:51:51Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

* Reducing changeset to TS-only * Update names; fix a few bugs * Test fixes * Add browser and server functions * Update ts-ignore comments; convert pie; break up TS defs * Convert const to func * Plot conversion * Tweaks to documentation * Fix deleted file * Fix type errors * Update yarn.lock * Address feedback

clintandrewhall added WIP Work in progress Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// labels Apr 15, 2019

clintandrewhall requested review from chrisdavies, crob611, monfera and w33ble April 15, 2019 16:18

clintandrewhall requested a review from a team as a code owner April 15, 2019 16:18

clintandrewhall changed the title ~~[WIP] Type and Add i18n to Canvas Functions~~ [WIP] Type and add i18n to Canvas Functions Apr 16, 2019

timroes mentioned this pull request Apr 18, 2019

[WIP] Add typings for interpreter function definitions #28954

Closed

10 tasks

clintandrewhall force-pushed the i18n-part-one branch from aafca71 to 778e592 Compare April 18, 2019 15:11

Reducing changeset to TS-only

622e56b

clintandrewhall force-pushed the i18n-part-one branch from 778e592 to 622e56b Compare April 22, 2019 16:30

clintandrewhall changed the title ~~[WIP] Type and add i18n to Canvas Functions~~ [WIP] Type Canvas Functions Apr 22, 2019

Update names; fix a few bugs

dcc20cf

Merge remote-tracking branch 'elastic' into i18n-part-one

4278f03

clintandrewhall added the v8.0.0 label Apr 26, 2019

Fix deleted file

f2dda3a

monfera reviewed Apr 26, 2019

View reviewed changes

clintandrewhall changed the title ~~[Canvas] Add Types to Functions~~ [WIP][Canvas] Add Types to Functions Apr 29, 2019

clintandrewhall removed the review label Apr 29, 2019

clintandrewhall mentioned this pull request Apr 29, 2019

[codemod][prereq] Convert Functions from arrow to function #35749

Merged

clintandrewhall added 3 commits April 29, 2019 12:53

Fix type errors

23e9567

Merge remote-tracking branch 'elastic' into i18n-part-one

e17ac26

Merge remote-tracking branch 'elastic' into i18n-part-one

05d50f9

clintandrewhall changed the title ~~[WIP][Canvas] Add Types to Functions~~ [Canvas] Add Types to Functions Apr 29, 2019

clintandrewhall added the review label Apr 29, 2019

Update yarn.lock

6264118

monfera reviewed May 1, 2019

View reviewed changes

Address feedback

57e9f75

monfera self-requested a review May 2, 2019 16:07

monfera approved these changes May 2, 2019

View reviewed changes

Merge remote-tracking branch 'elastic' into i18n-part-one

e5a76ba

clintandrewhall merged commit d2e083a into elastic:master May 2, 2019

clintandrewhall mentioned this pull request May 3, 2019

[7.x] [Canvas] Add Types to Functions (#35087) #36001

Merged

clintandrewhall deleted the i18n-part-one branch June 6, 2019 04:24

Uh oh!

Conversation

clintandrewhall commented Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Define and Enforce Function Arguments

Define and Enforce Context

Uh oh!

elasticmachine commented Apr 15, 2019

Uh oh!

elasticmachine commented Apr 15, 2019

💔 Build Failed

Uh oh!

lukasolson commented Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clintandrewhall commented Apr 15, 2019

Uh oh!

elasticmachine commented Apr 16, 2019

💔 Build Failed

Uh oh!

w33ble commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clintandrewhall commented Apr 17, 2019

Uh oh!

clintandrewhall commented Apr 17, 2019

Uh oh!

elasticmachine commented Apr 17, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Apr 18, 2019

💔 Build Failed

Uh oh!

w33ble commented Apr 18, 2019

Uh oh!

clintandrewhall commented Apr 22, 2019

Uh oh!

elasticmachine commented Apr 22, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Apr 22, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Apr 22, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Apr 26, 2019

💔 Build Failed

Uh oh!

monfera Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

clintandrewhall Apr 29, 2019

Choose a reason for hiding this comment

Uh oh!

monfera Apr 30, 2019

Choose a reason for hiding this comment

Uh oh!

clintandrewhall May 2, 2019

Choose a reason for hiding this comment

Uh oh!

monfera Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

clintandrewhall Apr 29, 2019

Choose a reason for hiding this comment

Uh oh!

clintandrewhall Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Apr 29, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Apr 29, 2019

💚 Build Succeeded

Uh oh!

monfera left a comment

Choose a reason for hiding this comment

clintandrewhall commented Apr 15, 2019 •

edited

Loading

lukasolson commented Apr 15, 2019 •

edited

Loading

w33ble commented Apr 17, 2019 •

edited

Loading

clintandrewhall Apr 29, 2019 •

edited

Loading