Extract src/core in a separate TS project by mshustov · Pull Request #76785 · elastic/kibana

mshustov · 2020-09-04T15:43:58Z

Summary

This PR moves src/core into a separate TS project. To do so, I had to make a few adjustments:

moved MethodKeysOf to @kbn/utility-types to able to use it in any package
added several hacks to remove core typescript dependency on data plugin & cli. These dependencies still exist in runtime. Dependency on data plugin should be removed in [core.savedObjects] Update esKuery imports to pull from new package instead of data plugin #55485
An issue to remove the dependency on CLI: Remove circular dependencies between CLI & core #76935

Measurements

TypeScript

node --max-old-space-size=6144 ./node_modules/.bin/tsc -p tsconfig.json --extendedDiagnostics --noEmit
branch

Files:                        5223
Lines:                      709462
Nodes:                     2346911
Identifiers:                799315

master

Files:                        5801
Lines:                      820489
Nodes:                     2711845
Identifiers:                920910

VSCode

Improved responsiveness when working in the src/core project.
Test case:

open src/core/server/index.ts file
click on CoreId type

branch
navigation takes ~5s
gif: https://recordit.co/7wd2wtuSIt

[Info  - 14:00:49.498] Starting TS Server 
[Trace  - 14:00:54.441] <syntax> Response received: references (29). Request took 6 ms. Success: true 
Result: {
    "refs": [
        {
            "file": "/Users/mikhailshustov/work/kibana/src/core/server/index.ts",
            "start": {
                "line": 93,
                "offset": 10
            },
            "end": {
                "line": 93,
                "offset": 16
            },
            "contextStart": {
                "line": 93,
                "offset": 1
            },
            "contextEnd": {
                "line": 93,
                "offset": 41
            },
            "lineText": "export { CoreId } from './core_context';",
            "isWriteAccess": true,
            "isDefinition": true
        }
    ],
    "symbolName": "CoreId",
    "symbolStartOffset": 10,
    "symbolDisplayString": "export CoreId"
}

master
navigation takes ~ 30sec.
gif: https://recordit.co/zFkNd1mGd6

[Info  - 13:56:46.789] Starting TS Server 
Trace  - 13:57:13.734] <syntax> Response received: references (60). Request took 5 ms. Success: true 
Result: {
    "refs": [
        {
            "file": "/Users/mikhailshustov/work/kibana-original/src/core/server/index.ts",
            "start": {
                "line": 93,
                "offset": 10
            },
            "end": {
                "line": 93,
                "offset": 16
            },
            "contextStart": {
                "line": 93,
                "offset": 1
            },
            "contextEnd": {
                "line": 93,
                "offset": 41
            },
            "lineText": "export { CoreId } from './core_context';",
            "isWriteAccess": true,
            "isDefinition": true
        }
    ],
    "symbolName": "CoreId",
    "symbolStartOffset": 10,
    "symbolDisplayString": "export CoreId"
}

Follow-ups:

bump query-string version to get rid of manually maintained types https://github.com/elastic/kibana/pull/76785/files#r483710091
find a way to import jest types to move DeeplyMockedKeys & MockedKeys to @kbn/utility-types

mshustov · 2020-09-04T15:46:49Z

src/core/server/legacy/legacy_service.ts

    if (this.coreContext.env.cliArgs.repl && process.env.kbnWorkerType === 'server') {
      // eslint-disable-next-line @typescript-eslint/no-var-requires
-      require('../../../cli/repl').startRepl(kbnServer);
+      require('./cli').startRepl(kbnServer);


a temporary hack to remove type dependencies between the core and cli. I'm going to enforce strict separation

created issue #76935

mshustov · 2020-09-04T15:50:51Z

src/legacy/server/kbn_server.d.ts

 // eslint-disable-next-line @kbn/eslint/no-restricted-paths
 import { UiPlugins } from '../../core/server/plugins';
 import { CallClusterWithRequest, ElasticsearchPlugin } from '../core_plugins/elasticsearch';
-import { UsageCollectionSetup } from '../../plugins/usage_collection/server';


removed dependency on plugin types

mshustov · 2020-09-04T15:54:06Z

src/core/typings.ts

+ * under the License.
+ */
+
+declare module 'query-string' {


I didn't want to include global typings in every package.
It seems there are no reason not to update query-string to the version shipped with type definitions. We had to roll back to the old version due to compatibility issues with IE11 #59633

Can't we have a ts project for /typings and import the reference?

we already have @kbn/utility-types for standard types, I'd rather avoid creating another utility package until it's unavoidable

mshustov · 2020-09-04T15:55:34Z

src/core/server/elasticsearch/client/mocks.ts

  (createInternalClientMock() as unknown) as ElasticsearchClientMock;

-interface ScopedClusterClientMock {
+export interface ScopedClusterClientMock {


required for TS to be able to reference the type in a declaration file

This reverts commit 79cec57.

mshustov · 2020-09-07T10:35:36Z

x-pack/package.json

    "@types/jsdom": "^16.2.3",
    "@types/json-stable-stringify": "^1.0.32",
    "@types/jsonwebtoken": "^7.2.8",
-    "@types/lodash": "^4.14.159",


got @types/lodash/ts3.1 error TS2300: Duplicate identifier error since both oss & x-pack types were included. I'd remove lodash from x-pack/package.json completely to rely on oss/package.json, but some plugins use import/no-extraneous-dependencies @elastic/kibana-operations

mshustov · 2020-09-08T13:19:42Z

src/core/typings.ts

+  export function extract(str: string): string;
+}
+
+type DeeplyMockedKeys<T> = {


Unfortunately, I didn't find a way to import MockInstance & Mocked interfaces from jest package. It didn't allow me to move DeeplyMockedKeys & MockedKeys interfaces to @kbn/utility-types, since global defined jest types conflict with mocha types when importing @kbn/utility-types in /test/ folder.

mshustov · 2020-09-08T13:41:22Z

examples/bfetch_explorer/tsconfig.json

-  "exclude": []
+  "exclude": [],
+  "references": [
+    { "path": "../../src/core/tsconfig.json" }


not a hard requirement, but it reduced the number of processed files

node --max-old-space-size=6144 ./node_modules/.bin/tsc -p examples/bfetch_explorer/tsconfig.json --extendedDiagnostics --noEmit

with references

Files: 876 Lines: 219410 Nodes: 789781 Identifiers: 286371

without references

Files: 1203 Lines: 272257 Nodes: 930313 Identifiers: 333766

elasticmachine · 2020-09-08T14:34:48Z

Pinging @elastic/kibana-platform (Team:Platform)

streamich

AppArch changes LGTM.

pgayvallet

LGTM for platform changes

pgayvallet · 2020-09-14T13:49:37Z

src/core/server/http/integration_tests/lifecycle_handlers.test.ts

+// eslint-disable-next-line @typescript-eslint/no-var-requires
+const pkg = require('../../../../../package.json');


The package file is also considered as an external dependency?

if you import, yes. it should gone when @kbn/utils is merged.

src/core/server/saved_objects/es_query.js

pgayvallet · 2020-09-14T14:38:37Z

src/core/typings.ts

+ * under the License.
+ */
+
+declare module 'query-string' {


Can't we have a ts project for /typings and import the reference?

spalger

LGTM, checked operations specific changes and ran it locally, all seems well

kibanamachine · 2020-09-15T09:56:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 5cd4bad

Build metrics

distributable file count

id	value	diff	baseline
default	45552	+5	45547
oss	27229	+5	27224
total		+10

History

💚 Build #74011 succeeded 744a7b1
💚 Build #73070 succeeded 2fba6fa
💔 Build #72928 failed c9b2342
💔 Build #72915 failed d62e1f3bc993b29d2385aa38de48cc28bcf2834f
💔 Build #72899 failed 795026e

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

* break dependency on data plugin TS code * move global typings to @kbn/utility-types * import types from @kbn/utility-types * remove type dependency on plugins * add intermediate js files to break dependency on outter TS code * temp type declaration for query-string * declare src/core project * export types to reference in the built d.ts files * reference core project * move jest types out of kbn/utility-types due to a clash with mocha types * fix wrong es_kuery path and ts project paths * reference core from packages consuming it * x-pack & oss should use the same lodash version * Revert "x-pack & oss should use the same lodash version" This reverts commit 79cec57. * use the same lodash version * fix @types/lodash TS2300: Duplicate identifier error * fix wrong imports * update docs * update docs * add a comment why file is needed # Conflicts: # packages/kbn-utility-types/index.ts # src/core/public/application/capabilities/capabilities_service.mock.ts # src/core/public/chrome/chrome_service.mock.ts

mshustov added 11 commits September 4, 2020 09:38

break dependency on data plugin TS code

871fa93

move global typings to @kbn/utility-types

2f8fe03

import types from @kbn/utility-types

58a6833

remove type dependency on plugins

053f33a

add intermediate js files to break dependency on outter TS code

3ee5024

temp type declaration for query-string

0afe83f

declare src/core project

b013a97

export types to reference in the built d.ts files

4d11218

reference core project

51f9a25

move jest types out of kbn/utility-types due to a clash with mocha types

4c720f0

Merge branch 'master' into src-core-ts-project

5109563

mshustov added chore Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Sep 4, 2020

mshustov commented Sep 4, 2020

View reviewed changes

mshustov added 7 commits September 4, 2020 18:02

fix wrong es_kuery path and ts project paths

51e45d5

reference core from packages consuming it

702edf8

x-pack & oss should use the same lodash version

79cec57

Revert "x-pack & oss should use the same lodash version"

568cf5d

This reverts commit 79cec57.

use the same lodash version

ffadf41

fix @types/lodash TS2300: Duplicate identifier error

a14a7ed

fix wrong imports

795026e

mshustov commented Sep 7, 2020

View reviewed changes

mshustov added 2 commits September 7, 2020 13:35

Merge branch 'master' into src-core-ts-project

3c5dd03

update docs

c9b2342

mshustov force-pushed the src-core-ts-project branch from d62e1f3 to c9b2342 Compare September 7, 2020 12:20

mshustov mentioned this pull request Sep 8, 2020

Move core config service to kbn/config package #76874

Merged

2 tasks

mshustov added 2 commits September 8, 2020 13:47

Merge branch 'master' into src-core-ts-project

d016606

update docs

2fba6fa

mshustov commented Sep 8, 2020

View reviewed changes

mshustov marked this pull request as ready for review September 8, 2020 14:34

mshustov requested review from a team as code owners September 8, 2020 14:34

mshustov added the v8.0.0 label Sep 10, 2020

Merge branch 'master' into src-core-ts-project

744a7b1

streamich approved these changes Sep 14, 2020

View reviewed changes

pgayvallet approved these changes Sep 14, 2020

View reviewed changes

spalger approved these changes Sep 14, 2020

View reviewed changes

mshustov added 2 commits September 15, 2020 10:14

Merge branch 'master' into src-core-ts-project

a4f8860

add a comment why file is needed

5cd4bad

mshustov merged commit e7d8ea3 into elastic:master Sep 15, 2020

mshustov deleted the src-core-ts-project branch September 15, 2020 10:41

mshustov mentioned this pull request Sep 15, 2020

[7.x] Extract src/core in a separate TS project (#76785) #77509

Merged

mshustov added the backported label Sep 16, 2020

pgayvallet mentioned this pull request Sep 16, 2020

clean core types before building types #77656

Closed

mshustov mentioned this pull request Sep 24, 2020

Switch to typescript project references and incremental builds #46773

Closed

7 tasks

mshustov mentioned this pull request Oct 22, 2020

explicitly export core mock types #81527

Closed

rudolf mentioned this pull request Dec 17, 2020

Move PublicMethodsOf into @kbn/utility-types #46699

Closed

		// eslint-disable-next-line @typescript-eslint/no-var-requires
		const pkg = require('../../../../../package.json');

Conversation

mshustov commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Measurements

TypeScript

VSCode

Follow-ups:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshustov Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshustov Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 8, 2020

Uh oh!

streamich left a comment

Choose a reason for hiding this comment

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Sep 15, 2020

💚 Build Succeeded

Build metrics

distributable file count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mshustov commented Sep 4, 2020 •

edited

Loading

mshustov Sep 4, 2020 •

edited

Loading

mshustov Sep 8, 2020 •

edited

Loading