-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Conversation
docs/development/typescript.md
Outdated
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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:
- 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
- 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); |
ack: will review tomorrow |
docs/development/typescript.md
Outdated
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. |
There was a problem hiding this comment.
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
``` | ||
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Co-authored-by: Josh Dover <[email protected]>
…docuement-ts-project-refa
There was a problem hiding this 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.
docs/development/typescript.md
Outdated
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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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. 😞
@rudolf added #79446 as an example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 |
@stacey-gammon converted newly added file. see |
* document ts project references * Apply suggestions from code review Co-authored-by: Josh Dover <[email protected]> * 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 <[email protected]>
* 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) ...
* document ts project references * Apply suggestions from code review Co-authored-by: Josh Dover <[email protected]> * 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 <[email protected]> Co-authored-by: Josh Dover <[email protected]>
part of #46773
adds docs for the solution teams to migrate plugins to TS project references