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

[Meta] Migrate plugins to TS references for es_ui CODEOWNERS plugins #89321

Closed
TinaHeiligers opened this issue Jan 26, 2021 · 14 comments · Fixed by #89472, #89488, #89622 or #90713
Closed

[Meta] Migrate plugins to TS references for es_ui CODEOWNERS plugins #89321

TinaHeiligers opened this issue Jan 26, 2021 · 14 comments · Fixed by #89472, #89488, #89622 or #90713
Labels
chore release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0

Comments

@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Jan 26, 2021

Summary

Now that the Kibana build system supports using TypeScript project references (#46773), the es_ui team need help migrating their plugins. This is part of the overall effort tracked at #80508.

plugin migration status
cross_cluster_replication
index_lifecycle_management
grokdebugger
index_management
license_management
painless_lab
remote_clusters
rollup
snapshot_restore
upgrade_assistant
watcher
ingest_pipelines

Migration order:

  1. plugins ready to migrate (no blockers):
    - watcher Migrates watcher to a TS project ref #89622
    - snapshot_restore TS project refs: Migrates snapshot_restore to a TS Project #89653
    - painless_lab Converts painlessLab to a TS project reference #89626
    - license_management Migrates license_management to a TS project ref #89472
    - grokdebugger TS project refs: Migrates grokdebugger #89652
    - ingest_pipelines Migrates ingest_pipelines to a TS project ref #89505
    - index_management Migrates index_management & runtime_fields to TS project refs #89809 (Circular dependency removed)
    - upgrade_assistant -> (Ts project refs: Migrates Upgrade Assistant to a TS project #89949)
  2. plugins blocked by circular dependencies
    - index_management Migrates index_management & runtime_fields to TS project refs #89809
    • index_lifecycle_management (depends on index_management)
    • remote_clusters (depends on index_management)
    • rollup (depends on index_pattern_management Migrate indexPatternManagement to TS project ref #89759 & index_management)
    • cross_cluster_replication (depends on remote_clusters & index_management)

Blockers (if any)

Blockers: ⚠️ Circular dependency between infra <--> apm <--> infra for
cross_cluster_replication, index_lifecycle_management, index_management, remote_clusters, rollup

Acceptance criteria

The telemetry-related plugins can be referred to using a project reference, which means each...

  • has a tsconfig.json file which conforms to the documented form
  • references the dependencies
  • emits type definitions for its public API
  • compiles in tsc's build mode

Related information

@TinaHeiligers
Copy link
Contributor Author

TinaHeiligers commented Jan 26, 2021

Blocked plugins and dependencies

index_management (5) (DONE)

Dependencies

- [ ] apm (#81003)

index_lifecycle_management (12)

Dependencies

- [ ] apm (#81003)

remote_clusters (12)

Dependencies

- [ ] apm (#81003)

rollup (12)

Dependencies

- [ ] apm (#81003)

cross_cluster_replication (13)

Dependencies

- [ ] apm (#81003)

@TinaHeiligers
Copy link
Contributor Author

TinaHeiligers commented Jan 26, 2021

@restrry the watcher plugin has a plain json file that's being imported in a few places. For example:

// x-pack/plugins/watcher/tests_client_integration
import defaultWatchJson from '../public/application/models/watch/default_watch.json';

With the following tsconfig.json:

//x-pack/plugins/watcher/tsconfig.json
{
  "extends": "../../../tsconfig.base.json",
  "compilerOptions": {
    "composite": true,
    "outDir": "./target/types",
    "emitDeclarationOnly": true,
    "declaration": true,
    "declarationMap": true
  },
  "include": [
    "common/**/*",
    "public/**/**/**/*",
    "server/**/*",
    "tests_client_integration/**/*",
    "__fixtures__/**/*",
    "../../typings/**/*"
  ],
  "references": [
    { "path": "../../../src/core/tsconfig.json" },
    { "path": "../licensing/tsconfig.json" },
    { "path": "../features/tsconfig.json" },
    { "path": "../../../src/plugins/home/tsconfig.json"},
    { "path": "../../../src/plugins/charts/tsconfig.json"},
    { "path": "../../../src/plugins/data/tsconfig.json"},
    { "path": "../../../src/plugins/management/tsconfig.json"},
    { "path": "../../../src/plugins/es_ui_shared/tsconfig.json"},
    { "path": "../../../src/plugins/kibana_react/tsconfig.json"},
  ]
}

I get the following error that I can't seem to get around:

➜  kibana git:(watcher-to-ts-ref) ✗ ./node_modules/.bin/tsc -b x-pack/plugins/watcher
x-pack/plugins/watcher/tests_client_integration/watch_create_json.test.ts:11:30 - error TS6307: File '/Users/christianeheiligers/Projects/kibana/x-pack/plugins/watcher/public/application/models/watch/default_watch.json' is not listed within the file list of project '/Users/christianeheiligers/Projects/kibana/x-pack/plugins/watcher/tsconfig.json'. Projects must list all files or use an 'include' pattern.

11 import defaultWatchJson from '../public/application/models/watch/default_watch.json';
                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


Found 1 error.

Please forgive me if you've answered this before but how would one get around this?
I'm trying to resolve the issue by creating a new file and converting the json object to a js const. There are a few failing tests now though and I'm not sure how much we should be fiddling with the es_ui team's code or if I can fix them.

@TinaHeiligers
Copy link
Contributor Author

@cjcenizal perhaps you can help me out with the watcher migration issue above? The TL;DR is that defaultWatchJson is declared in a .json file and typescript complains. When I change the defaultWatchJson to a js object, there are a few failing unit tests and the behavior is not the same. I'm not familiar with the watcher code and don't want to 'mess' with it too much. Thanks!

@cjcenizal
Copy link
Contributor

@TinaHeiligers Thanks for helping us with this and for the ping! I'll take a look at Watcher. In the meantime, do you want to move it to the bottom of your list so you get to it last? I might also suggest prioritizing license management, since according to the meta issue it's blocking ML.

@TinaHeiligers
Copy link
Contributor Author

@cjcenizal thank you for taking a look at watcher!

I'll prioritize license_management and will ping in this issue if there are similar problems.

In the mean time, we need to remove the dependency between feet and index_management.
index_management is a low level plugin used by many other plugins and ATM, it depends on fleet. The only reason that there is a dependency on fleet is to generate a link to the fleet app.

Could someone from the @elastic/es-ui team either look into removing that dependency as a priority?
Thanks!

@cjcenizal
Copy link
Contributor

@TinaHeiligers

we need to remove the dependency between fleet and index_management.

Can you help me understand this need?

The only reason that there is a dependency on fleet is to generate a link to the fleet app

Are you aware of an alternative way to do this? If fleet isn't enabled, we don't want to render a link to it. What's the correct way to assert against whether it's enabled or not without declaring it an optional dependency?

@TinaHeiligers
Copy link
Contributor Author

TinaHeiligers commented Jan 27, 2021

@cjcenizal, @restrry asked me to reach out about removing the dependency on fleet.
Migrating index_management to a TS project is blocked by a circular dependency between infra and apm.
The dependency chain is: index_management --> fleet --> infra --> apm
If we can remove the dependency on fleet, we (hopefully) won't need to wait for the circular dependency to be resolved (from my understanding of the request but I might be wrong here)

As for alternative ways of checking if fleet is enabled, is there a config setting you can check?

@cjcenizal
Copy link
Contributor

@TinaHeiligers Thanks for explaining. I asked around and it sounds like we're following best practices in index_management. You mentioned the circular dependency lies between infra and apm. I think we should focus on solving that problem directly rather than trying to work around the downstream effects.

@TinaHeiligers
Copy link
Contributor Author

TinaHeiligers commented Jan 28, 2021

Blockers for migrating upgrade_assistant:
1. Find an alternative for reading the current version merged

// x-pack/plugins/upgrade_assistant/common/version.ts
import pkg from '../../../../package.json' // we're importing kibana/package.json

2. Find a way to reference xpackMocks: Resolved with duplicating the mock used in one particular test.

3. Convert AssistanceData from .json to .ts:

4. Convert fakeDeprecations from .json to .ts:

TIL: one needs to explicitly declare .json in the tsconfig.json as follows:

// tsconfig.json
...
  },
  "include": [
    "common/**/*",
    // have to declare *.json explicitly due to https://github.com/microsoft/TypeScript/issues/25636
    "public/**/*.json",
    "server/**/*"
  ],
...

cc @elastic/es-ui

@elastic/kibana-core Does anyone have advice on how to handle 1 & 2? I can get around 3 & 4.

@cjcenizal
Copy link
Contributor

@alisonelizabeth Could you help Tina out with 1 and 2 above?

@alisonelizabeth
Copy link
Contributor

I can look into this on Friday.

@mshustov
Copy link
Contributor

mshustov commented Jan 29, 2021

@TinaHeiligers what's the problem with json files? The same as here?

// have to declare *.json explicitly due to https://github.com/microsoft/TypeScript/issues/25636
"server/**/*.json",

@TinaHeiligers
Copy link
Contributor Author

@restrry oh wow, I didn't know we could do that!
Thanks

@TinaHeiligers TinaHeiligers reopened this Jan 29, 2021
@alisonelizabeth
Copy link
Contributor

@TinaHeiligers Just a quick update on UA - I can read the current version from PluginInitializerContext.env.packageInfo.version instead of package.json. I will also handle xpackMocks. I'll tag you when I have a PR up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment