-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Use multiple TypeScript projects #16430
Use multiple TypeScript projects #16430
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/56759 |
a3a615a
to
7bd3efc
Compare
The last commit (d826ec0,
Before this PR, |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
d826ec0
to
38f7cad
Compare
Type checking now runs with
This also caught some errors in the It takes ~30 seconds to run, which is acceptable. |
cc @bradzacher @JoshuaKGoldberg I assume that this is just because we have 140 projects, but I'm pinging you just in case you want to take a look :) Linting is already slow for us, so this is not a blocker. It's weird however that when linting the same code, it breaks when linting it as multiple projects but works when using a single giant one. EDIT EDIT2: |
I was going to suggest Given ESLint doesn't tell plugins when it's done with files, perhaps this is a case of every single file being left open. I tried to take a pprof profile of the run to see where all the memory is going, but that segfaults... |
This shouldn't be affected by using multiple projects, right? |
It would; each project is its own "thing" internally, so the project service is actually spinning up multiple projects (same as the editor) as files are opened by ts-eslint. But if they're never closed, it's kinda like if you opened every file in the editor and never closed them again. But within reason, that shouldn't be so massively different than I'm also checking to see if this is the |
This PR is ready for review! Some info:
Also, I suggest reviewing filtering away JSON files since the diff is big and GitHub's UI struggles. |
2b0c02a
to
6cf9532
Compare
@@ -1,5 +1,3 @@ | |||
/// <reference path="../../../lib/third-party-libs.d.ts" /> | |||
|
|||
import type { Token as JSToken, JSXToken } from "js-tokens"; |
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.
This was unnecessary and causing an invalid .d.ts file
type === "UpdateExpression" | ||
? NodeDescriptions.UpdateExpression[String(prefix) as "true" | "false"] | ||
: NodeDescriptions[type]; | ||
const toNodeDescription = (node: NodeWithDescription) => |
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.
This was causing an invalid .d.ts file, because TS drops the @ts-expect-error
I'm not sure if having |
|
We can now also migrate to |
Jake has it right - there's sadly no infra in place for eslint to tell us when it's done with a file. There's also nothing to differentiate the persistent usecase (IDEs) from the "single-run" usecase (CLIs) which means we have to try and guess and fall back to "keep everything just in case". 140 is a lot of packages so it's unsurprising that it's OOMing. Essentially you're hoping that TS will share things as much as possible. But in reality even with the project service TS has to create a bunch of There's sadly nothing we can do about this. We've discussed with the ESLint team and submitted proposals but have been rejected thus far. The best solution is to isolate things and spawn one eslint process per package. This way you physically isolate things and thus manually cleanup the memory when a package is done linting. I see you essentially did this already. Note that something like nx does this automatically heh IIRC. I have an old prototype CLI which was essentially a wrapper around eslint and did said splitting across child processes (typescript-eslint/typescript-eslint#4359) which was designed to do this generically and showed some good results in testing. It might be time to revive that 🤔 |
/* This file is automatically generated by scripts/generators/tsconfig.js */ | ||
{ | ||
"compilerOptions": { | ||
"paths": { |
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.
Interesting, i'm not sure why you are using ts paths here and not npm workspaces with exports
also using lib like wireit might help with eslint issue
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... don't know 😅 It's what we were already doing before this PR. I guess it might be because we don't mark type-only dependencies as dependencies of packages, so the node_modules/workspaces structure is not complete for what TS needs.
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 the node_modules/workspaces structure is not complete for what TS needs.
I didn't got this point, I think keeping what you have now but with using ts project refs and drop the ts-paths file makes more sense I guess...
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.
Avoiding paths would be great if you can; doing so effectively lies to the compiler about module layouts. Sometimes that's okay (especially for bundled projects), but I'm sorta surprised that this wouldn't "just work" if this repo is already a monorepo with package linking.
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.
If I remove paths TS complains that it cannot find the type definitions inside node_modules
(probably because we store them in a separate directory) -- I'll keep using paths
because TS is able to automatically map from the source files to their corresponding .d.ts
.
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'll experiment in a separate PR.
scripts/parallel-tsc/tsc.js
Outdated
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 worth noting that if you're not doing a --fix
run then you can actually pass the ts.Program
instance and reuse the types for the lint run.
https://typescript-eslint.io/packages/parser#programs
This would net you a modest speedup by skipping one "typecheck" cycle.
Note - you'd need to switch from invoking tsc
via a child process to directly using the typescript APIs to do this though.
Thought it was worth mentioning in case you want speeeeed
@bradzacher Thanks for the explanation! Yes, I was indeed expecting typescript-eslint or the ts service to indeed share types across projects rather than re-computing them every time. I also just realized that yes we are now spawning multiple processes, but we are doing it serially rather than in parallel. This might be easy way for us to save some time :) |
Fixes #1, Fixes #2
With Project References,
tsc
is able to only type-check "projects" whose contents changed, without having to re-check the whole monorepo every time.Usually, when using a monorepo, there is a 1:1 mapping between packages and TypeScript projects. In our case this doesn't work, because Project References do not support cycles. Instead, we generate one project per strongly connected component in our packages graph: we currently have only one SCC1, and its
tsconfig.json
is inpackages/babel-helper-plugin-utils/tsconfig.json
.All the
tsconfig.json
files are not manually maintained (except for/tsconfig.base.json
), but they are generated vianode scripts/generators/tsconfig.js
.See #16430 (comment) for how this impacts our development workflows.
Old description
I also experimented with running TypeScript on multiple cores rather than in just one.
tsc
doesn't support it by default, but you can usenode scripts/parallel-tsc/tsc.js .
(or passing the path of any other project) to do it. It seems to be ~40% faster than plaintsc
on a MacBook M2 with 8 cores (but for some reason only 6 are being used).This PR makes running a full type checking significantly slower (this will affect CI), but it makes incremental builds significantly faster (up to 10x, depending on which package is modified, and this will affect local development). This are the results I'm getting now (but I was getting different results this morning, unless I hallucinate them):
rm -rf dts tsconfig.tsbuildinfo && yarn tsc -b .
takes 31s (both with and withoutskipLibCheck
)make clean-ts && yarn tsc -b .
takes 5m 22sskipLibCheck: true
,make clean-ts && yarn tsc -b .
takes 1min 53smake clean-ts && node scripts/parallel-tsc/tsc.js .
takes 2m 42sskipLibCheck: true
,make clean-ts && node scripts/parallel-tsc/tsc.js .
takes 1min 16sI believe the reason
skipLibCheck: true
significantly improves performance is that it lets TS avoid re-validating all the.d.ts
files it generated for the intermediate projects. I will mark this PR as ready once we can get theskipLibCheck: true
results while still validating our manually-written.d.td
files.@jakebailey Here is the branch :) Please feel free to ask any question that might help you, if you are going to take a look
Footnotes
(
@babel/helper-plugin-utils <> @babel/code-frame <> @babel/compat-data <> @babel/core <> @babel/generator <> @babel/helper-check-duplicate-nodes <> @babel/helper-compilation-targets <> @babel/helper-environment-visitor <> @babel/helper-fixtures <> @babel/helper-function-name <> @babel/helper-hoist-variables <> @babel/helper-module-imports <> @babel/helper-module-transforms <> @babel/helper-plugin-test-runner <> @babel/helper-simple-access <> @babel/helper-split-export-declaration <> @babel/helper-string-parser <> @babel/helper-transform-fixture-test-runner <> @babel/helper-validator-identifier <> @babel/helper-validator-option <> @babel/helpers <> @babel/highlight <> @babel/parser <> @babel/template <> @babel/traverse <> @babel/types
, with 26 packages out of ~150, but I'm looking into reducing its size by splitting some packages across multiple projects to reduce cycles). ↩