Establish pattern for typing legacy plugins by joshdover · Pull Request #26045 · elastic/kibana

joshdover · 2018-11-21T20:11:15Z

Summary

This adds a new common pattern for adding type definitions to legacy plugins and the KbnServer itself.

Now plugins that wish to use TypeScript can import our customized Server type from src/server/kbn_server to get a type that has definitions supplied for each of the objects on server.plugins (eventually).

Plugins (core_plugins, x-pack, and external) will be able to use our exported types by importing the Legacy namespace:

import { Legacy } from 'kibana'

function registerRoute(server: Legacy.Server) {
  // has full type defintions for this plugin
  const { callWithRequest } = server.plugins.elasticsearch.getCluster('admin');  
}

// Could reference plugin types directly
type ESPlugin = Legacy.Plugins.elasticsearch.ElasticsearchPlugin;

elasticmachine · 2018-11-21T20:11:45Z

Pinging @elastic/kibana-platform

stacey-gammon · 2018-12-13T16:18:38Z

Any idea on the ETA of this PR? I'd like to use KibanaConfig :)

stacey-gammon · 2018-12-13T16:22:38Z

There is a top level typings folder too, I think types is new in this PR? I think this will end up being confusing. Maybe they can be merged?

https://github.com/elastic/kibana/tree/master/typings

hm, no there isn't, so actually types folder must be a build artifact, hence the gitignore? Still a bit confusing then I think... but I also am not fully aware of everything that is going on with the types setup, and that typings folder is pretty new so maybe there was just a disconnect.

The types directory is built from yarn build:types which gets run on yarn kbn bootstrap. In master it goes to the target/types directory but in this PR I changed it to types.

The reason I changed it to not be target is that the build wipes out that directory before building and I was previously trying to import from it which was failing Typescript build for x-pack. Let me confirm, but I think after some of the other changes I made, I can put this back the way it was before.

Confirmed, I'll put this back to pulling from the target directory to reduce confusion. Thanks for catching this!

elasticmachine · 2018-12-13T23:24:08Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sorenlouv

Changes to APM look good 👍

epixa · 2018-12-14T12:44:01Z

Why import type information differently depending on whether the plugin is internal or not?

azasypkin · 2018-12-14T15:07:45Z

+/**
+ * All exports from TS source files (where the implementation is actualy done in TS).
+ */
+export * from './target/types/type_exports';


question: btw, I just realized that we may need to add one more Project to src/dev/build/tasks/transpile_typescript_task.js with tsconfig.types.json and run it as the first one, so that plugins that depend on generated types can be successfully built/transpiled during build? We have just one now, src/plugins/test, but it's excluded from build. It's semi out-of-scope for this PR so feel free to ignore that :)

Ok, I've updated the build to build this project, but in the repo root rather than the build root. The reason for this is we build the typescript files from the repo root in x-pack so the types will need to be present there. I've also added an alias so you can now do this in any x-pack code:

import { Legacy } from 'kibana'

azasypkin · 2018-12-14T15:23:05Z

@@ -0,0 +1,391 @@
+/*


question: is this file crafted by you or we have copied it from somewhere? Just checking whether I should review it or not :)

It's written by me. Due to the way we use strings in callWithRequest to lookup methods on the elasticsearch.js Client I had to write all of those method calls with the appropriate params and return types from '@types/elasticsearch'

Got it, thanks!

azasypkin · 2018-12-14T15:26:25Z

+import { ElasticsearchPlugin } from '../legacy/core_plugins/elasticsearch';
+
+export interface KibanaConfig {
+  get<T = any>(key: string): T;


question: should it rather be T = unknown?

azasypkin · 2018-12-14T16:12:26Z


 import { GraphQLSchema } from 'graphql';
-import { Request, ResponseToolkit, Server } from 'hapi';
+import { Request, ResponseToolkit, Server } from 'src/server/kbn_server';


question: why can't we use relative paths here and point to the root index.d.ts with Legacy namespace?

See comment above, you can now do import { Legacy } from 'kibana'

joshdover · 2018-12-14T18:40:38Z

@epixa I've changed the build process so now they can all import the same way, directly from 'kibana'.

elasticmachine · 2018-12-14T19:03:16Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

joshdover · 2018-12-14T20:30:42Z

retest

elasticmachine · 2018-12-14T21:11:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin

The overall shape of this PR and the part related to the new platform look good to me, thanks! The only part I haven't looked closely at is the the Elasticsearch client types, but we can improve them along the way if/when needed.

azasypkin · 2018-12-17T17:26:36Z

@@ -0,0 +1,48 @@
+/*


question: just curious, why not index.d.ts as it was before? I believe there is even a convention to rely on index.d.ts from the project root if types isn't specified in package.json (not that we shouldn't set it explicitly, but still).

TypeScript compiler doesn't support using the compilerOptions.paths feature to reference the index.d.ts file, but it works when using a different name. Using this option was necessary to support import ... from 'kibana' for x-pack and core packages.

elasticmachine · 2018-12-17T18:51:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Add legacy types and export them for plugins * Add support for core_plugins to import from 'kibana'

joshdover · 2018-12-17T19:56:43Z

6.x: 16a900a

* Add legacy types and export them for plugins * Add support for core_plugins to import from 'kibana'

joshdover added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v7.0.0 v6.6.0 labels Nov 21, 2018

joshdover changed the title ~~[WIP] Establish pattern for typing legacy plugins~~ Establish pattern for typing legacy plugins Nov 21, 2018

joshdover commented Nov 21, 2018

View reviewed changes

Comment thread src/core_plugins/elasticsearch/index.d.ts Outdated