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

Document ts project references setup #78586

Merged
merged 13 commits into from
Oct 6, 2020
54 changes: 54 additions & 0 deletions docs/development/typescript.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
# Typescript
Although this is not a requirement, we encourage if all new code is developed in [Typescript](https://www.typescriptlang.org/).

- [Project references](#project-references)
- [Caveats](#caveats)
- [Prerequisitions](#prerequisitions)
- [Implementation](#implementation)

## Project references
Kibana has crossed the 1.5m LoC mark. The current situation creates some scaling problems when the default out-of-the-box setup stops working. As a result, developers suffer from slow project compilation and IDE unresponsiveness. As a part of [Developer Experience project](https://github.com/elastic/kibana/projects/63), we are migrating our tooling to use built-in TypeScript features addressing the scaling problems - [project references](https://www.typescriptlang.org/docs/handbook/project-references.html) & [incremental buils](https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-4.html#faster-subsequent-builds-with-the---incremental-flag). In a nutshell - instead of compiling the whole Kibana codebase at once, this setup enforces splitting the code base into independent projects that form a directed acyclic graph (DAG) that allows the TypeScript compiler(`tsc`) to apply several advanced optimizations:
mshustov marked this conversation as resolved.
Show resolved Hide resolved
- every project emits *public* interfaces in the form of `d.ts` type declaration
- created`d.ts` type declarations are used whenever a referenced project is imported in a depending project
- it makes it possible to determine which project needs rebuilding when the source code has changed to use a more aggressive caching strategy.
More details are available in the [official docs](https://www.typescriptlang.org/docs/handbook/project-references.html)
mshustov marked this conversation as resolved.
Show resolved Hide resolved

### Caveats
This architecture imposes several limitations to which we must comply:
- projects cannot have circular dependencies. Even though the Kibana platform doesn't support circular dependencies between Kibana plugins, TypeScript does allow circular type imports between files. So in theory, you might face a problem when migrating to the TS project references so you will have to remove this circular dependency.
- a project must emit its type declaration. It's not always possible to generate a type declaration if the compiler cannot reference a type. There are two basic cases:
mshustov marked this conversation as resolved.
Show resolved Hide resolved
1. Your plugin exports a type inferring an internal type declared in Kibana codebase. In this case, you'll have to either export an internal type or to declare an exported type explicitly.
2. Your plugin exports something inferring a type from a 3rd party library that doesn't export this type. To fix the problem, you have to declare the exported type manually.

### Prerequisitions
Since `tsc` doesn't support circular references, the migration order does matter. You can migrate your plugin only when all the plugin dependencies already have migrated. It creates the situation when low-level plugins (such as `data` or `kibana_react`) have to migrate first.
Copy link
Contributor Author

@mshustov mshustov Sep 28, 2020

Choose a reason for hiding this comment

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

It would be useful to add a script showing a plugin dependency tree. I didn't manage to find an issue - I can create one. Should we prioritize it? Manual migration order tracking is not cool.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could probably be solved by the same tool as #78162 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it would help to understand all the existing deps between plugins. But:

  1. It's no clear when Create tool to identify and visualize circular imports between plugins #78162 is taken in work by the @elastic/kibana-operations
  2. A simple script visualizing a plugin tree from the plugin discovery would do the trick as well. As a bonus, we could highlight all the projects already migrated to TS project refs.

The main problem is the plugin discovery is tightly coupled with the platform runtime. We might have to either extract it in or borrow @kbn/optimizer implementation

const plugins = findKibanaPlatformPlugins(options.pluginScanDirs, options.pluginPaths);

mshustov marked this conversation as resolved.
Show resolved Hide resolved

### Implementation
mshustov marked this conversation as resolved.
Show resolved Hide resolved
- make sure all the plugins listed as dependencies in `kibana.json` file have migrated to TS project references.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think a numbered list makes more sense here since you will generally start at the top and work down

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to revert it in 6efc968 to the unordered list. asciidoctor fails validation with

18:06:10 INFO:build_docs:asciidoctor: WARNING: developer/best-practices/typescript.asciidoc: line 35: list item index: expected 2, got 1
18:06:10 INFO:build_docs:asciidoctor: WARNING: developer/best-practices/typescript.asciidoc: line 59: list item index: expected 2, got 1
18:06:10 INFO:build_docs:asciidoctor: WARNING: developer/best-practices/typescript.asciidoc: line 60: list item index: expected 3, got 1
18:06:10 INFO:build_docs:asciidoctor: WARNING: developer/best-practices/typescript.asciidoc: line 61: list item index: expected 4, got 1
18:06:10 INFO:build_docs:asciidoctor: WARNING: developer/best-practices/typescript.asciidoc: line 62: list item index: expected 5, got 1

I tried 1., 2., etc. style as well, but the numeration started from 1 after source block. 😞

- add `tsconfig.json` in the root folder of your plugin.
```json
{
"extends": "../../../tsconfig.base.json",
"compilerOptions": {
"composite": true,
"outDir": "./target/types",
"emitDeclarationOnly": true,
"declaration": true,
"declarationMap": true
},
"include": [
// add all the folders containg files to be compiled
],
"references": [
{ "path": "../../core/tsconfig.json" },
// add references to other TypeScript projects your plugin dependes on
]
}
```
If your plugin imports a file not listed in `include`, the build will fail with the next message `File ‘…’ is not listed within the file list of project …’. Projects must list all files or use an 'include' pattern.`
- build you plugin `./node_modules/.bin/tsc -b src/plugins/my_plugin`. Fix errors if `tsc` cannot generate type declarations for your project.
- make sure the `target/types` folder doesn’t contain files not belonging to your project. Otherwise, it means your plugin includes files from another project by mistake.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an easy way to detect this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can write a script to make sure all files in outDir covered by patterns in includes. However, if code from another plugin is compiled, it means that this plugin has been listed in include. I'm leaning towards removing this note from the doc. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's not expected to be a common situation, then let's either remove it or move it to a "troubleshooting" section.

- add your project reference to `include` property of `tsconfig.refs.json`
mshustov marked this conversation as resolved.
Show resolved Hide resolved
- add your plugin to `include` property and plugin folder to `exclude` property of the `tsconfig.json` it used to belong to (for example, for `src/plugins/**` it's `tsconfig.json`; for `x-pack/plugins/**` it’s `x-pack/tsconfig.json`).
mshustov marked this conversation as resolved.
Show resolved Hide resolved
- list the reference to your newly created project in all the Kibana `tsconfig.json` files that could import your project: `tsconfig.json`, `test/tsconfig.json`, `x-pack/tsconfig.json`, `x-pack/test/tsconfig.json`. And in all the plugin-specific `tsconfig.refs.json` for dependent plugins.
- you can measure how your changes affect `tsc` compiler performance with `node --max-old-space-size=4096 ./node_modules/.bin/tsc -p tsconfig.json --extendedDiagnostics --noEmit`. Compare with **master** branch.