-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add TS projects for src/plugins & x-pack/plugins #78440
Conversation
log.info('Building TypeScript projects refs...'); | ||
await execa(require.resolve('typescript/bin/tsc'), ['-b', 'tsconfig.refs.json']); | ||
log.debug(`Building TypeScript projects refs for ${projectPath}...`); | ||
await execa(require.resolve('typescript/bin/tsc'), ['-b', projectPath]); |
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.
to avoid hardcoding x-pack/tsconfig.refs.json
@@ -80,7 +80,8 @@ export async function runTypeCheckCli() { | |||
process.exit(); | |||
} | |||
|
|||
await buildRefs(log); | |||
await buildRefs(log, 'tsconfig.refs.json'); | |||
await buildRefs(log, 'x-pack/tsconfig.refs.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.
I hardcoded the path here since this script is meant to be used in dev only. @elastic/kibana-operations any problems with this?
@@ -13,6 +13,7 @@ | |||
], | |||
"exclude": [], | |||
"references": [ | |||
{ "path": "../../src/core/tsconfig.json" } | |||
{ "path": "../../src/core/tsconfig.json" }, | |||
{ "path": "../../src/plugins/kibana_react/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.
it's not necessary, as we don't build the project with tsc
, but it reduces the number of processed files:
with refs
Files: 884
Lines: 219774
Nodes: 789226
Identifiers: 286286
without refs
Files: 1214
Lines: 271616
Nodes: 928343
Identifiers: 333268
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.
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.
It might become hard to maintain all the references if we start having many small projects. Maybe we can set composite: true
in ./tsconfig.refs.json
and then all other projects that might need these references just contain a single reference to ./tsconfig.refs.json
instead of a reference for each project.
I didn't test this, but my understanding is that you can create such hierarchies of projects and TS will still be clever about what files it includes in the build instead of building the whole parent project.
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 didn't test this, but my understanding is that you can create such hierarchies of projects and TS will still be clever about what files it includes in the build instead of building the whole parent project.
Tested just to make sure it doesn't work: TS cannot build a DAG since a project references itself and other project dependent on it.
It might become hard to maintain all the references if we start having many small projects.
I declared the reference to kibana_react
only for projects with an explicit dependency on kibanaReact
plugin in kibana.json
. Indeed, kibana_react
might me compiled as a transitive dependency (you can run tsc
with --traceResolution
flag to learn how a file was included). But we will resolve the over-compilation problem when switching all the plugins to TypeScript project refs, so I think it's okay for now.
Pinging @elastic/kibana-platform (Team:Platform) |
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/Transform 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.
I pulled and played with kibana_react
, didn't notice any issues. Rebuild on changes works, node scrtips/type_check
works, IDE is responsive with suggestions and errors. Hard to assess responsiveness improvements during such testing.
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. Just a suggestion @restrry : I would probably create a buildAllRefs
function on src/dev/typescript/build_refs.ts
which builds all the refs projects (even if hardcoded) and use it at src/dev/typescript/run_type_check_cli.ts
instead
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]async chunks size
distributable file count
page load bundle size
History
To update your PR or re-run it, just comment with: |
@restrry yes, I think it's more manageable like this! We do something similar for kbn/pm projects |
* bump query-string version to remove manual type definitions * remove manual type declaration * add kibana_utils tsconfig * add refs to kibana_utils tsconfig * add kibana_utils to the project list * add kibana_react project * add support for x-pack/tsconfig.refs.json * add ts project for x-pack licensing plugins * add glob for ts projects in src/plugins & x-pack/plugins * add refs to projects in examples * fix ref paths in x-pack/test * address mistic comments Co-authored-by: Elastic Machine <[email protected]> # Conflicts: # src/plugins/kibana_utils/kibana.json
* bump query-string version to remove manual type definitions * remove manual type declaration * add kibana_utils tsconfig * add refs to kibana_utils tsconfig * add kibana_utils to the project list * add kibana_react project * add support for x-pack/tsconfig.refs.json * add ts project for x-pack licensing plugins * add glob for ts projects in src/plugins & x-pack/plugins * add refs to projects in examples * fix ref paths in x-pack/test * address mistic comments Co-authored-by: Elastic Machine <[email protected]> # Conflicts: # src/plugins/kibana_utils/kibana.json
Summary
Add TS projects for:
I'm going to document migration path in the following PR.
I'd appreciate if @elastic/kibana-app-arch tested the changes and how they impact IDE responsiveness when working inside
kibana_utils
&kibana_react
plugins.Results
OSS project
node --max-old-space-size=6144 ./node_modules/.bin/tsc -p tsconfig.json --extendedDiagnostics --noEmit
branch
master
x-pack project
node --max-old-space-size=6144 ./node_modules/.bin/tsc -p x-pack/tsconfig.json --extendedDiagnostics --noEmit
branch
master