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

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Sep 28, 2020

part of #46773
adds docs for the solution teams to migrate plugins to TS project references

@mshustov mshustov added chore v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 docs labels Sep 28, 2020
@mshustov mshustov added this to Pending Review in kibana-core [DEPRECATED] via automation Sep 28, 2020
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 mshustov changed the title Document ts project references Document ts project references setup Sep 28, 2020
@mshustov mshustov requested a review from a team September 28, 2020 13:45
@joshdover
Copy link
Contributor

ack: will review tomorrow

docs/development/typescript.md Outdated Show resolved Hide resolved
docs/development/typescript.md Outdated Show resolved Hide resolved
docs/development/typescript.md Outdated Show resolved Hide resolved
docs/development/typescript.md Outdated Show resolved Hide resolved
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

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 ?

docs/development/typescript.md Outdated Show resolved Hide resolved
```
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.

Copy link
Contributor

@rudolf rudolf left a comment

Choose a reason for hiding this comment

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

I think these instructions could benefit from a fully worked out example since there's quite a lot of places to edit. Alternatively we could link to the PR that moved kibana_react (but it has some other changes that wouldn't be necessary) or maybe we can link to the PR of the first team that moved their plugin.

Since `tsc` doesn't support circular project references, the migration order does matter. You can migrate your plugin only when all the plugin dependencies already have migrated. It creates a situation where commonly used plugins (such as `data` or `kibana_react`) have to migrate first.

### Implementation
- 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. 😞

docs/development/typescript.md Outdated Show resolved Hide resolved
docs/development/typescript.md Outdated Show resolved Hide resolved
@mshustov
Copy link
Contributor Author

mshustov commented Oct 5, 2020

@rudolf added #79446 as an example
@stacey-gammon Should I merge the current file with https://github.com/elastic/kibana/blob/master/TYPESCRIPT.md ? What is a better place for the docs? docs/developer as per #41833 (comment) ?

@mshustov mshustov requested a review from rudolf October 5, 2020 12:42
Copy link
Contributor

@rudolf rudolf left a comment

Choose a reason for hiding this comment

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

@rudolf added #79446 as an example

Awesome!

@stacey-gammon
Copy link
Contributor

@restrry, good question. Merging with https://github.com/elastic/kibana/blob/master/TYPESCRIPT.md looks like a good idea. I'd migrate them both to asciidoc, put inside docs/developer/best_practices, and link to from docs/developer/best_practices/index.asciidoc.

@mshustov
Copy link
Contributor Author

mshustov commented Oct 5, 2020

@stacey-gammon converted newly added file. see docs/developer/best-practices/typescript.asciidoc

@mshustov mshustov merged commit 6f98372 into elastic:master Oct 6, 2020
kibana-core [DEPRECATED] automation moved this from Pending Review to Done (7.10) Oct 6, 2020
@mshustov mshustov deleted the docuement-ts-project-refa branch October 6, 2020 06:47
mshustov added a commit to mshustov/kibana that referenced this pull request Oct 6, 2020
* document ts project references

* Apply suggestions from code review

Co-authored-by: Josh Dover <me@joshdover.com>

* capitalize all the bullet points

* address @rudolf comments

* add a reference to example

* fix identation

* convert  into asciidoc

* fix numeration

* fix asciidoctor failures

* cleanup typescript.asciidoc

* back to unordered list. asciidoctor fails validation

Co-authored-by: Josh Dover <me@joshdover.com>
@mshustov mshustov moved this from In Progress to Done in Kibana developer experience [NOTICE TO CLOSE] Oct 6, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 6, 2020
* master: (85 commits)
  Refactor attribute service (elastic#78414)
  [APM] Add default message to alerts. (elastic#78930)
  [Discover] Modify columns and sort when switching index pattern (elastic#74336)
  Document ts project references setup (elastic#78586)
  build all ts refs in single kbn:bootstrap (elastic#79438)
  [TSVB] Allow string fields on value count aggregation (elastic#79267)
  [SECURITY SOLUTION] Investigate EQL signal in timeline (elastic#79049)
  [Fleet] Add loading spinner to page headers (elastic#79568)
  [Security Solution][Resolver] Resolver query panel load more (elastic#79160)
  Add type row to monitor detail page. (elastic#79556)
  Remove license refresh from setup (elastic#79518)
  [docker] add reporting fonts (elastic#74806)
  [SECURITY_SOLUTION][ENDPOINT] Add info about trusted apps to the page subtitle + create flyout (elastic#79558)
  Trim Hash value before validating it (elastic#79545)
  Warn users when security is not configured (elastic#78545)
  update copy styling (elastic#79313)
  Update dependency @elastic/charts to v23.1.1 (elastic#78459)
  Introduce geo-threshold alerts (elastic#76285)
  elastic#76920 Show base breadcrumb when there is an error booting the app (elastic#79571)
  remove react-intl from kibana and keep it inside only i18n package (elastic#78956)
  ...
mshustov added a commit that referenced this pull request Oct 6, 2020
* document ts project references

* Apply suggestions from code review

Co-authored-by: Josh Dover <me@joshdover.com>

* capitalize all the bullet points

* address @rudolf comments

* add a reference to example

* fix identation

* convert  into asciidoc

* fix numeration

* fix asciidoctor failures

* cleanup typescript.asciidoc

* back to unordered list. asciidoctor fails validation

Co-authored-by: Josh Dover <me@joshdover.com>

Co-authored-by: Josh Dover <me@joshdover.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported chore docs release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0
Development

Successfully merging this pull request may close these issues.

None yet

4 participants