Migrate App services plugins to TS projects #87294

mshustov · 2021-01-05T12:43:37Z

Summary

Migrated @elastic/kibana-app-services plugins to the TS projects.
Migrated save_objects and features plugins as several plugins depend on them.
Moved code from examples and fixtures to appropriate plugins to remove unnecessary dependencies.
Fixed script searching for plugins ready to be migrated to TS projects.

Impact

tsc --extendedDiagnostics
before:
OSS:

Files:                         6345
Lines:                       646052
Nodes:                      2212898
Identifiers:                 747346

x-pack:

Files:                        19629
Lines:                      2338294
Nodes:                      8150557
Identifiers:                2528494

after
OSS

Files:                         4961
Lines:                       538175
Nodes:                      1872436
Identifiers:                 638292

x-pack

Files:                        19005
Lines:                      2255307
Nodes:                      7910176
Identifiers:                2447677

mshustov · 2021-01-05T16:12:28Z

x-pack/plugins/features/server/oss_features.test.ts

@@ -5,7 +5,8 @@
 */

 import { buildOSSFeatures } from './oss_features';
-import { featurePrivilegeIterator } from '../../security/server/authorization';
+// @ts-expect-error
+import { featurePrivilegeIterator } from './feature_privilege_iterator';


workaround to remove the circular dependency between features and security. @legrego wouldn't it be more appropriate to move this privilege assertion into the security plugin test? https://github.com/elastic/kibana/pull/87294/files#diff-bb6d1fa1f497224ebef5f42ca080dc5baadd274b96fd8ebc99652071fd5a4a77R64

Strictly speaking, yes! There was a reason we did it this way originally, but I'm struggling to remember why. It may have involved not wanting to export buildOSSFeatures from the features plugin, but looking at it now, it's no different than exporting featurePrivilegeIterator from the security plugin 🤷.

I can add a task to address this in followup

elasticmachine · 2021-01-05T16:28:22Z

Pinging @elastic/kibana-core (Team:Core)

spalger

Operations related changes LGTM

Bamieh

LGTM

streamich

The changes LGTM, but would be also good to get a second pair of 👀 on this from somebody who is more familiar with data plugin.

mshustov · 2021-01-06T14:09:05Z

@elasticmachine merge upstream

pgayvallet

LGTM. A few questions

pgayvallet · 2021-01-06T13:50:33Z

src/dev/run_find_plugins_ready_migrate_to_ts_refs.ts

-    return get(JSON.parse(content), 'compilerOptions.composite', false);
+    return get(JSON5.parse(content), 'compilerOptions.composite', false);


Was there an issue with the default JSON parser?

tsconfig.json is not strict JSON https://www.reddit.com/r/typescript/comments/8na5vb/tsconfigjson_isnt_strict_json_what_now/

src/plugins/data/common/index_patterns/index_patterns/fixtures/logstash_fields.js

pgayvallet · 2021-01-06T14:03:09Z

src/plugins/ui_actions/tsconfig.json

+    { "path": "../../core/tsconfig.json" },
+    { "path": "../expressions/tsconfig.json" },
+    { "path": "../kibana_utils/tsconfig.json" },
+    { "path": "../kibana_react/tsconfig.json" },


I see you changed the expressions import to be import type, but I wonder if we should add expressions as a requiredBundle in ui_action's kibana.json file? I'm afraid of these inconsistencies between tsconfig.json and kibana.json, wdyt? Theorically, a plugin shouldn't import code (or even just types) from a plugin it doesn't have dependency on, right?

It's not an obligatory requirement since requiredBundle is for runtime deps. I might add it to requiredBundle, but there is no way to enforce it. One way to do so is to generate tsconfig.json file from kibana.json when all the plugins migrate to TS projects. @spalger WDYT?

We verify that people don't have unnecessary requiredBundles by ensuring that each is actually used, but we aren't able to do that if we start including type-only dependencies in the list. I think it's better to keep the requiredBundles array as small as possible and that includes removing type-only dependencies as well as bundles which are no longer required because usage was removed.

pgayvallet · 2021-01-06T14:07:46Z

x-pack/plugins/data_enhanced/tsconfig.json

+  "include": [
+    "common/**/*",
+    "public/**/*",
+    "public/autocomplete/providers/kql_query_suggestion/__fixtures__/*.json",


Why is "public/autocomplete/providers/kql_query_suggestion/__fixtures__/*.json" explicitly needed for json files? I don't see any similar inclusion in

kibana/x-pack/tsconfig.json

Line 3 in 1ec2f1d

"include": ["mocks.ts", "typings/**/*", "plugins/**/*", "tasks/**/*"],

It seems to be a bug in tsc when building composite project with resolveJsonModule: true microsoft/TypeScript#25636
I will add a comment to the tsconfig.json file.

…ervices-ts-refs

lizozom · 2021-01-06T15:46:36Z

src/plugins/data/common/index_patterns/index_patterns/fixtures/logstash_fields.js

+    // name               esType        |      |      |metadata       | subType
+    ['bytes', 'long', true, true, { count: 10 }],
+    ['ssl', 'boolean', true, true, { count: 20 }],
+    ['@timestamp', 'date', true, true, { count: 30 }],


This fixture still exists in src/fixtures and is being used in other plugins, any reason why we started duplicating them?

Yes, fixtures folder doesn't have an owner. This file is used by data and discover plugins only. If you want to keep them in sync, we might need to consider exporting them from the data plugin.

So what would be the rule of thumb of what goes into src/fixtures?

what goes into src/fixtures?

nothing. it should be removed eventually. global folders won't exist when we move to domain-based folder structure #71566

lizozom

data plugin changes LGTM

copy of fixtures into the plugin
adding the tsconfig.json file

mshustov · 2021-01-06T19:23:07Z

@elasticmachine merge upstream

kibanamachine · 2021-01-06T21:36:34Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6d887b7

Metrics [docs]

Distributable file count

id	before	after	diff
`default`	47260	48192	+932
`oss`	27710	27713	+3

History

💔 Build #97326 failed 1bde8d5
💔 Build #97274 failed 62c1403
💚 Build #97022 succeeded b4f9b28
💚 Build #97011 succeeded a39e6c0
💔 Build #96997 failed eda816b

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* migrate expressions to ts project refs * bfetch to ts project * ui_actions to ts project * move fitures to data plugins * add data ts project * remove outdated ts-expect-error * add data to x-pack tsconfigs * navigation to ts project * cleanup licensing tsconfig * saved_objects to ts project * embeddable to ts project * ui_actions_enhanced to ts project * embeddable_enhanced to ts project * features to ts project * data_enhanced to ts project refs * fix i18n check * fix find_plugins_ready_to_migrate_to_ts_refs script * add a comment for bug ignoring json for composite projects Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

mshustov added 7 commits December 29, 2020 10:41

migrate expressions to ts project refs

575f615

bfetch to ts project

60ace67

ui_actions to ts project

ec63c40

move fitures to data plugins

63a41f5

add data ts project

e3eb995

Merge branch 'master' into app-services-ts-refs

ed123e1

remove outdated ts-expect-error

6f89403

mshustov mentioned this pull request Jan 5, 2021

Migrate data plugin and dependencies to TS project references #87081

Closed

8 tasks

mshustov added 9 commits January 5, 2021 13:50

add data to x-pack tsconfigs

a37458a

navigation to ts project

8cce148

cleanup licensing tsconfig

1cde72d

saved_objects to ts project

d693686

embeddable to ts project

e9e65e7

ui_actions_enhanced to ts project

9fea2ce

embeddable_enhanced to ts project

b017b13

features to ts project

e9d7fa0

data_enhanced to ts project refs

eda816b

mshustov commented Jan 5, 2021

View reviewed changes

mshustov added chore release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0 Team:AppServices Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Jan 5, 2021

mshustov added this to In Progress in Kibana developer experience [NOTICE TO CLOSE] Jan 5, 2021

fix i18n check

a39e6c0

mshustov marked this pull request as ready for review January 5, 2021 16:28

mshustov requested review from a team as code owners January 5, 2021 16:28

fix find_plugins_ready_to_migrate_to_ts_refs script

b4f9b28

mshustov requested a review from a team as a code owner January 5, 2021 16:51

botelastic bot added Feature:Embedding Embedding content via iFrame Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) labels Jan 5, 2021

spalger approved these changes Jan 5, 2021

View reviewed changes

TinaHeiligers mentioned this pull request Jan 5, 2021

[Usage Collection & Telemetry] Migrate to TS project references #81017

Closed

7 tasks

legrego mentioned this pull request Jan 5, 2021

Remove the circular dependency between features and security #87388

Closed

Bamieh approved these changes Jan 6, 2021

View reviewed changes

streamich approved these changes Jan 6, 2021

View reviewed changes

Merge branch 'master' into app-services-ts-refs

62c1403

pgayvallet approved these changes Jan 6, 2021

View reviewed changes

Merge remote-tracking branch 'origin/app-services-ts-refs' into app-s…

11e89da

…ervices-ts-refs

lizozom reviewed Jan 6, 2021

View reviewed changes

add a comment for bug ignoring json for composite projects

1bde8d5

lizozom approved these changes Jan 6, 2021

View reviewed changes

Merge branch 'master' into app-services-ts-refs

6d887b7

mshustov merged commit 69e2c38 into elastic:master Jan 7, 2021

mshustov deleted the app-services-ts-refs branch January 7, 2021 08:30

This was referenced Jan 7, 2021

[7.x] Migrate App services plugins to TS projects (#87294) #87604

Merged

[Core] Migrate the remaining plugins to TS project refs #85386

Closed

mshustov added the backported label Jan 7, 2021

smith mentioned this pull request Jan 7, 2021

[APM] Migrate Kibana plugins (that depend on APM) to TS project references #81003

Closed

mshustov mentioned this pull request Jan 25, 2021

Migrate bfetch to TS Project References #86040

Closed

TinaHeiligers mentioned this pull request Jan 26, 2021

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

Closed

mshustov moved this from In Progress to Done in Kibana developer experience [NOTICE TO CLOSE] Feb 7, 2021

mshustov removed this from Done in Kibana developer experience [NOTICE TO CLOSE] Feb 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate App services plugins to TS projects #87294

Migrate App services plugins to TS projects #87294

mshustov commented Jan 5, 2021 •

edited

Loading

mshustov Jan 5, 2021

legrego Jan 5, 2021

elasticmachine commented Jan 5, 2021

spalger left a comment

Bamieh left a comment

streamich left a comment

mshustov commented Jan 6, 2021

pgayvallet left a comment

pgayvallet Jan 6, 2021

mshustov Jan 6, 2021

pgayvallet Jan 6, 2021 •

edited

Loading

mshustov Jan 6, 2021 •

edited

Loading

spalger Jan 6, 2021

pgayvallet Jan 6, 2021

mshustov Jan 6, 2021

lizozom Jan 6, 2021 •

edited

Loading

mshustov Jan 6, 2021

lizozom Jan 6, 2021

mshustov Jan 6, 2021 •

edited

Loading

lizozom left a comment

mshustov commented Jan 6, 2021

kibanamachine commented Jan 6, 2021

		return get(JSON.parse(content), 'compilerOptions.composite', false);
		return get(JSON5.parse(content), 'compilerOptions.composite', false);

Migrate App services plugins to TS projects #87294

Migrate App services plugins to TS projects #87294

Conversation

mshustov commented Jan 5, 2021 • edited Loading

Summary

Impact

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jan 5, 2021

spalger left a comment

Choose a reason for hiding this comment

Bamieh left a comment

Choose a reason for hiding this comment

streamich left a comment

Choose a reason for hiding this comment

mshustov commented Jan 6, 2021

pgayvallet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

mshustov Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizozom Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

lizozom left a comment

Choose a reason for hiding this comment

mshustov commented Jan 6, 2021

kibanamachine commented Jan 6, 2021

💚 Build Succeeded

Metrics [docs]

Distributable file count

History

mshustov commented Jan 5, 2021 •

edited

Loading

pgayvallet Jan 6, 2021 •

edited

Loading

mshustov Jan 6, 2021 •

edited

Loading

lizozom Jan 6, 2021 •

edited

Loading

mshustov Jan 6, 2021 •

edited

Loading