-
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
Introduce TS incremental builds & move src/test_utils to TS project #76082
Conversation
"@testing-library/jest-dom" | ||
] | ||
// overhead is too significant | ||
"incremental": false, |
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.
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
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.
So we'll need to break up x-pack into multiple projects to get benefits here, correct?
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.
@joshdover yes, as per #46773 (comment)
src/core/tsconfig.json
Outdated
"extends": "../../tsconfig.base.json", | ||
"compilerOptions": { | ||
// "composite": true, | ||
"outDir": "./target", |
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.
not used at the moment, going to use it in the next PR. Should I remove it?
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 guess comment it out, just in case that PR gets delayed for some reason.
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.
Telemetry changes LGTM!
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.
ML latest changes LGTM
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.
AppArch changes LGTM.
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.
APM changes look good!
This comment has been minimized.
This comment has been minimized.
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.
Thank you!
P.S.
This PR will break a script used by the Security Solution team. #76422 fixes it.
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.
KibanaApp changes LGTM, just test files were modified, didn't checkout
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.
ES UI changes look good, spotted an unrelated bug that we can fix in a follow up PR.
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.
- 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.
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.
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'; |
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.
tried importing this from a plugin, seems to not cause any eslint error. Most of our rules are targeting src/core/{server|public}
{ | ||
"extends": "../../tsconfig.json" | ||
} |
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.
Not sure from the diff if the file is now empty or was removed?
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.
removed
"compilerOptions": { | ||
"tsBuildInfoFile": "../../../../build/tsbuildinfo/security_solution/cypress", |
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.
Just to understand, why this wasn't added in some of the config files (such as packages/kbn-monaco/tsconfig.json
)?
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.
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.
💚 Build SucceededBuild metricsasync chunks size
page load bundle size
oss distributable file count
distributable file count
History
To update your PR or re-run it, just comment with: |
…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
…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
…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) ...
* 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) ...
…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
Summary
This PR:
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-flagsrc/test_utils
dependency on the core anddata
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.htmlyarn kbn bootstrap
&node scripts/type_check
. This is a trade-off of project references usageBecause 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 tonode scripts/type_check
as well to ensure the latest changes are reflected in d.ts files. The performance penalty of runningtsc -b
is insignificant due toincremental
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
master
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:
master:
time node scripts/type_check --project tsconfig.json
the current branch
first pass
second pass
master
first pass
second pass
Follow-ups
src/core
src/plugins/kibana_utils