Skip to content
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

Expression typings: Canvas to OSS #37438

Merged
merged 7 commits into from
May 30, 2019

Conversation

lukeelmers
Copy link
Member

@lukeelmers lukeelmers commented May 29, 2019

Note: This PR replaces #36960, which I had to close after a botched merge. 🙄

Summary

Moves the Canvas Expression Function types that Clint worked on over to OSS so they can be exported from the interpreter for use by other plugins.

Usage / Testing

import { ExpressionFunction } from 'plugins/interpreter';
// ^^ From x-pack this currently needs to be done via absolute import, unfortunately:
// import { ExpressionFunction } from 'src/legacy/core_plugins/interpreter/public';

const name = 'myfunc';

interface Context {
  type: 'whatever'; // in most cases you'll be importing Context from elsewhere
}

interface Arguments {
  foo: boolean | null;
}

interface Return {
  type: 'whatever'; // in most cases you'll be importing Return from elsewhere
}

export const myfunc = (): ExpressionFunction<typeof name, Context, Arguments, Return> => ({
  // function definition
});

@lukeelmers
Copy link
Member Author

@clintandrewhall @ppisljar @chrisdavies Sorry fellas, I did something strange in my merge on the original PR so had to open this new one. Nothing has changed so far other than a few updates to address the feedback from Clint.

Feel free to chime in with anything else that's missing. We have both been looking at Clint's testing branch #37300 and trying to sort out the CI issues there... seems to have to do with failed module imports and (hopefully) nothing to do with the implementation itself.

@lukeelmers lukeelmers added :AppArch Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) review labels May 29, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@clintandrewhall
Copy link
Contributor

As of now, switching the imports to 'plugins/interpreter/types' does not resolve. Let me know if there's something else I should test.

@lukeelmers
Copy link
Member Author

lukeelmers commented May 30, 2019

I updated the PR so imports could be done from the top-level (plugins/interpreter rather than plugins/interpreter/types) to remain in line with new platform conventions.

However, it seems that the build fails still when trying to resolve webpack plugins/* aliases within ts files. I've tried updating the x-pack tsconfig but had no luck. The relative imports from within x-pack also fail during build. For now what seemed to work (at least when building locally) was absolute imports from src/legacy/core_plugins/interpreter/public.

I'm optimistic this will fix the CI failures on #37300, but it is something we should come back to fix in a subsequent PR so that webpack aliases can work properly within x-pack.

Copy link
Contributor

@clintandrewhall clintandrewhall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this works in #37300, I can approve this on behalf of :Canvas. Great working with you on this!

},
help: i18n.translate('interpreter.functions.esaggs.help', { defaultMessage: 'Run AggConfig aggregation' }),
help: i18n.translate('interpreter.functions.esaggs.help', {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to take a look at how we're enforcing i18n in Canvas... because if you use the same methodology, we can move those types to OSS as well, (in a future PR).

https://github.com/elastic/kibana/blob/master/x-pack/plugins/canvas/canvas_plugin_src/strings/functions/function_help.ts#L79...L97

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Will check it out

@lukeelmers lukeelmers added v8.0.0 v7.3.0 release_note:skip Skip the PR/issue when compiling release notes and removed review labels May 30, 2019
@lukeelmers
Copy link
Member Author

I'll go ahead and merge so that we can get #37300 in. There are a few follow-up items we'll need to address in subsequent PRs:

  • Merge [Canvas] Interpreter type conversion #37300 so we clean up the duplicate code & switch over our imports
  • Colocate interfaces for types with the definitions in the interpreter
  • Consider how to deal with help text; investigate Canvas implementation as @clintandrewhall suggests above
  • Consider re-exporting these from the top-level of plugins/data instead (where this will ultimately live in new platform), instead of from plugins/interpreter
  • Switch the interpreter functions over to TS

@lukeelmers lukeelmers merged commit 100b088 into elastic:master May 30, 2019
@lukeelmers lukeelmers deleted the feat/oss-interpreter-types branch May 30, 2019 19:37
@elasticmachine
Copy link
Contributor

💔 Build Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.3.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants