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

Introduce TS incremental builds & move src/test_utils to TS project #76082

Merged
merged 32 commits into from
Sep 3, 2020

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Aug 27, 2020

Summary

This PR:

  • creates base tsconfig that other project will extend
  • introduces incremental builds for all TS projects but x-pack.
    TypeScript 3.4 introduces a new flag called --incremental which tells TypeScript to save information about the project graph from the last compilation. The next time TypeScript is invoked with --incremental, it will use that information to detect the least costly way to type-check and emit changes to your project. from https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-4.html#faster-subsequent-builds-with-the---incremental-flag
  • removes src/test_utils dependency on the core and data plugin code to prevent circular refs.TS project references do not support circular imports. More about Project references https://www.typescriptlang.org/docs/handbook/project-references.html
  • add type declaration build step during yarn kbn bootstrap & node scripts/type_check. This is a trade-off of project references usage Because dependent projects make use of .d.ts files that are built from their dependencies, you’ll either have to check in certain build outputs or build a project after cloning it before you can navigate the project in an editor without seeing spurious errors.
    from https://www.typescriptlang.org/docs/handbook/project-references.html#caveats-for-project-references
    Because tsc is not used to build nor to run Kibana, I had to add the build step to node scripts/type_check as well to ensure the latest changes are reflected in d.ts files. The performance penalty of running tsc -b is insignificant due to incremental build usage. @elastic/kibana-operations let me know if you envision any problems with the current approach.
    Worth noting that IDE (tested on WebStorm & VSCode) detect interface changes without rebuilding TS projects
    http://g.recordit.co/GtI8g4GqMr.gif

Measurements

With other project references

node ./node_modules/.bin/tsc -p src/core/tsconfig.json --extendedDiagnostics
the current branch

Files:                        3121
Lines:                      479324
Nodes:                     1625644
Identifiers:                562814

master

Files:                         3121
Lines:                       479118
Nodes:                      1625127
Identifiers:                 562643

Note that the number of processed Lines, Nodes, and Identifiers has decreased.

Type check

node scripts/type_check --project tsconfig.json --extendedDiagnostics

the current branch:

Files:                        5847
Lines:                      785862
Nodes:                     2642704
Identifiers:                900846

master:

Files:                        5852
Lines:                      786481
Nodes:                     2644345
Identifiers:                901353

time node scripts/type_check --project tsconfig.json

the current branch
first pass

real	1m3.162s
user	1m26.289s
sys	0m2.470s

second pass

real	0m10.656s
user	0m15.411s
sys	0m1.127s

master
first pass

real	0m40.846s
user	0m59.207s
sys	0m2.116s

second pass

real	0m39.864s
user	0m57.243s
sys	0m2.145s

Follow-ups

  • introduce project references for platform code under src/core
  • introduce project references for src/plugins/kibana_utils
  • document migration path for solution teams

@mshustov mshustov added chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.10.0 labels Aug 27, 2020
"@testing-library/jest-dom"
]
// overhead is too significant
"incremental": false,
Copy link
Contributor Author

@mshustov mshustov Aug 27, 2020

Choose a reason for hiding this comment

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

node scripts/type_check --project x-pack/tsconfig.json

incremental: true first pass

real	12m22.044s
user	12m47.693s
sys	0m20.303s

incremental: true second pass

real	1m16.779s
user	1m14.341s
sys	0m7.595s

incremental: true after changing a file

real	5m11.158s
user	4m46.419s
sys	0m12.713s

incremental: false

real	6m30.505s
user	6m34.917s
sys	0m14.650s

Copy link
Contributor

Choose a reason for hiding this comment

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

So we'll need to break up x-pack into multiple projects to get benefits here, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mshustov mshustov changed the title Move src/test_utils to separate ts project Introduce TS incremental builds & move src/test_utils to TS project Aug 31, 2020
"extends": "../../tsconfig.base.json",
"compilerOptions": {
// "composite": true,
"outDir": "./target",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not used at the moment, going to use it in the next PR. Should I remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess comment it out, just in case that PR gets delayed for some reason.

Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

Telemetry changes LGTM!

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

ML latest changes LGTM

Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

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

AppArch changes LGTM.

Copy link
Contributor

@smith smith left a comment

Choose a reason for hiding this comment

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

APM changes look good!

@oatkiller

This comment has been minimized.

Copy link
Contributor

@oatkiller oatkiller left a comment

Choose a reason for hiding this comment

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

Thank you!

P.S.
This PR will break a script used by the Security Solution team. #76422 fixes it.

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

KibanaApp changes LGTM, just test files were modified, didn't checkout

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

ES UI changes look good, spotted an unrelated bug that we can fix in a follow up PR.

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

  • add type declaration build step during yarn kbn bootstrap & node scripts/type_check.

I'm concerned about how this will work moving forward, but I think there is a very low risk of it causing problems right away and we can address issues as they pop up with regard to the caching/updating of types and the issues IDEs experience.

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -330,7 +330,7 @@ Cons:
To have access to Kibana TestUtils, you should create `integration_tests` folder and import `test_utils` within a test file:
```typescript
// src/plugins/my_plugin/server/integration_tests/formatter.test.ts
import * as kbnTestServer from 'src/test_utils/kbn_server';
import * as kbnTestServer from 'src/core/test_helpers/kbn_server';
Copy link
Contributor

Choose a reason for hiding this comment

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

tried importing this from a plugin, seems to not cause any eslint error. Most of our rules are targeting src/core/{server|public}

Comment on lines -1 to -3
{
"extends": "../../tsconfig.json"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure from the diff if the file is now empty or was removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

Comment on lines 7 to +8
"compilerOptions": {
"tsBuildInfoFile": "../../../../build/tsbuildinfo/security_solution/cypress",
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to understand, why this wasn't added in some of the config files (such as packages/kbn-monaco/tsconfig.json)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tsc emits it in outDir by default. For cases when we don't emit code or emit via declarationDir, an explicit tsBuildInfoFile path is required. Otherwise, the cache file will be written in the same folder when tsconfig.json is.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
enterpriseSearch 372.4KB +30.0B 372.4KB

page load bundle size

id value diff baseline
upgradeAssistant 64.7KB +30.0B 64.6KB

oss distributable file count

id value diff baseline
total 27303 +6 27297

distributable file count

id value diff baseline
total 45462 +6 45456

History

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

@mshustov mshustov merged commit e9e0ca3 into elastic:master Sep 3, 2020
@mshustov mshustov deleted the test-utils-ts-project branch September 3, 2020 12:20
mshustov added a commit to mshustov/kibana that referenced this pull request Sep 3, 2020
…lastic#76082)

* move test_helpers to the core

* create base tsconfig

* all tsconfigs use the base one

* use test_helpers exposed from the src/core

* move getFieldFormatsRegistry to data plugin

* add test_utils project

* compile types after checkout

* add a stub for platform tsconfig.json

* fix broken import

* fix broken path to the base config

* set tsBuildInfoFile for project without outDir

* do not commit tsbuildinfo file

* do not check output d.ts files

* fix type error

* use separate config to build types

* rollback changes to include paths

* mute import zone error

* rename files to avoid references to tsd

* do not use tsd for type tests

* include all ts files in project

* run buildRefs before type check to ensure the latest version

* store tsbuildinfo locally

* update paths to base config

* comment out core/tsconfig.json

* remove ui path

* fix wrong tsbuildinfo path
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 3, 2020
…nes/processors-forms-k-s

* 'master' of github.com:elastic/kibana: (216 commits)
  [Ingest Manager] Split Registry errors into Connection & Response (elastic#76558)
  [Security Solution] add an excess validation instead of the exact match (elastic#76472)
  Introduce TS incremental builds & move src/test_utils to TS project  (elastic#76082)
  fix bad merge (elastic#76629)
  [Newsfeed] Ensure the version format when calling the API (elastic#76381)
  remove server_extensions mixin (elastic#76606)
  Remove legacy applications and legacy mode (elastic#75987)
  [Discover] Fix sidebar element focus behavior when adding / removing columns (elastic#75749)
  Replace FetchOptions with ISearchOptions (elastic#76538)
  Move streams utils to the core  (elastic#76397)
  [Ingest Manager] Wording & linking improvements (elastic#76284)
  remove legacy kibana plugin (elastic#76064)
  [Form lib] Fix regression on field not being validated after reset to its default value. (elastic#76379)
  Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal (elastic#76482)
  [APM] Service maps layout enhancements (elastic#76481)
  [Security Solution][Detection Engine] Remove RuleTypeSchema in favor of Type for TypeScript (elastic#76586)
  [Security Solution][Exceptions] - Updates enum schema and tests (elastic#76544)
  Index Pattern class - remove toJSON and toString (elastic#76246)
  skip failing suite (elastic#76581)
  Split up and clarify Enterprise Search codeowners (elastic#76580)
  ...

# Conflicts:
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processor_settings_fields.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/foreach.tsx
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 3, 2020
…oleysens/kibana into ingest-pipelines/processors-forms-k-s

* 'ingest-pipelines/processors-forms-k-s' of github.com:jloleysens/kibana: (216 commits)
  [Ingest Manager] Split Registry errors into Connection & Response (elastic#76558)
  [Security Solution] add an excess validation instead of the exact match (elastic#76472)
  Introduce TS incremental builds & move src/test_utils to TS project  (elastic#76082)
  fix bad merge (elastic#76629)
  [Newsfeed] Ensure the version format when calling the API (elastic#76381)
  remove server_extensions mixin (elastic#76606)
  Remove legacy applications and legacy mode (elastic#75987)
  [Discover] Fix sidebar element focus behavior when adding / removing columns (elastic#75749)
  Replace FetchOptions with ISearchOptions (elastic#76538)
  Move streams utils to the core  (elastic#76397)
  [Ingest Manager] Wording & linking improvements (elastic#76284)
  remove legacy kibana plugin (elastic#76064)
  [Form lib] Fix regression on field not being validated after reset to its default value. (elastic#76379)
  Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal (elastic#76482)
  [APM] Service maps layout enhancements (elastic#76481)
  [Security Solution][Detection Engine] Remove RuleTypeSchema in favor of Type for TypeScript (elastic#76586)
  [Security Solution][Exceptions] - Updates enum schema and tests (elastic#76544)
  Index Pattern class - remove toJSON and toString (elastic#76246)
  skip failing suite (elastic#76581)
  Split up and clarify Enterprise Search codeowners (elastic#76580)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 3, 2020
* master: (340 commits)
  [data.search.SearchSource] Remove legacy ES client APIs. (elastic#75943)
  [release notes] automatically retry on Github API 5xx errors (elastic#76447)
  [es_ui_shared] Fix eslint exhaustive deps rule (elastic#76392)
  [i18n] Integrate 7.9.1 Translations (elastic#76391)
  [APM] Update aggregations to support script sources (elastic#76429)
  [Security Solution] Refactor Network Top Countries to use Search Strategy (elastic#76244)
  Document security settings available on ESS (elastic#76513)
  [Ingest Manager] Add input revision to the config send to the agent (elastic#76327)
  [DOCS] Identifies cloud settings for Monitoring (elastic#76579)
  [DOCS] Identifies Cloud settings in Dev Tools (elastic#76583)
  [Ingest Manager] Better default value for fleet long polling timeout (elastic#76393)
  [data.indexPatterns] Fix broken rollup index pattern creation (elastic#76593)
  [Ingest Manager] Split Registry errors into Connection & Response (elastic#76558)
  [Security Solution] add an excess validation instead of the exact match (elastic#76472)
  Introduce TS incremental builds & move src/test_utils to TS project  (elastic#76082)
  fix bad merge (elastic#76629)
  [Newsfeed] Ensure the version format when calling the API (elastic#76381)
  remove server_extensions mixin (elastic#76606)
  Remove legacy applications and legacy mode (elastic#75987)
  [Discover] Fix sidebar element focus behavior when adding / removing columns (elastic#75749)
  ...
mshustov added a commit that referenced this pull request Sep 3, 2020
…76082) (#76632)

* move test_helpers to the core

* create base tsconfig

* all tsconfigs use the base one

* use test_helpers exposed from the src/core

* move getFieldFormatsRegistry to data plugin

* add test_utils project

* compile types after checkout

* add a stub for platform tsconfig.json

* fix broken import

* fix broken path to the base config

* set tsBuildInfoFile for project without outDir

* do not commit tsbuildinfo file

* do not check output d.ts files

* fix type error

* use separate config to build types

* rollback changes to include paths

* mute import zone error

* rename files to avoid references to tsd

* do not use tsd for type tests

* include all ts files in project

* run buildRefs before type check to ensure the latest version

* store tsbuildinfo locally

* update paths to base config

* comment out core/tsconfig.json

* remove ui path

* fix wrong tsbuildinfo path
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.