Don't use "composite": true in tsc (until it supports cycles) #13242
Don't use "composite": true in tsc (until it supports cycles) #13242nicolo-ribaudo merged 8 commits intobabel:mainfrom
"composite": true in tsc (until it supports cycles) #13242Conversation
"composite": true in tsconfig.json until it supports cycles"composite": true in tsconfig.json
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/45785/ |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit c7c0b2b:
|
"composite": true in tsconfig.json"composite": true in tsconfig.json
There was a problem hiding this comment.
I do not think there is a need to include peerDependencies in project references. In most cases - it does not need to be included. And everything currently in the codebase works just fine if to include only dependencies referenced in source code and dependencies in package.json (I already did that in #11578). For @babel/core, it is also ok, in most cases - for usages currently in the codebase, it does not cause circular dependency.
However, there are indeed cases that currently have a good reason for circular dependency, recent example with helper which is not yet merged - #13122 (helper using NodePath from @babel/traverse used by @babel/traverse itself)
Not sure what is the best way to solve cases like this, my gut feeling - there should be a good way to avoid circular dependency, either making that helper a part of @babel/traverse or maybe better - splitting part of @babel/traverse into a separate package.
Generally - I would prefer not to have circular dependencies, I think this would enforce better separation of concerns. And having composite build in ts is a good bonus. But that might be a big change, and currently is not very clear how big.
About the change itself - added some comments, but should be fine.
This will likely make type checks a bit slower, but I guess it is unlikely to be very noticeable(as I can understand, ts server is good about updating its state incrementally).
I think, it is better to come back to question about having circular dependencies after converting everything to TypeScript and improving type coverage, which will provide better understanding about the options and effort required to "fix" all circular dependencies. Also, maybe microsoft/TypeScript#33685 would be an option to solve this sometime in the future.
| } | ||
|
|
||
| function mapToDts(packageName) { | ||
| return packageName.replace(/\bpackages\b/, "dts"); |
There was a problem hiding this comment.
What about eslint and codemods? - I guess eventually it should also be migrated to typescript
| input, | ||
| plugins: [rollupDts()], | ||
| onwarn(warning, warn) { | ||
| if (warning.code !== "CIRCULAR_DEPENDENCY") warn(warning); |
There was a problem hiding this comment.
Why there is a warning about it? Can it cause issue when publishing d.ts files?
There was a problem hiding this comment.
It doesn't cause issues because types are hoisted. It's only a problem when two modules depend on each other and they export/import non-hoistable declarations:
// a.js
import { b } from "b";
export let a = b;import { a } from "a";
export let b = a;
Yes, that was exactly the PR that made me prepare this one. Also, I think that for now we don't have many circular dependencies similar to that one, but as soon as we start improving our internal type annotations they will happen more often. At least for now, I would like to avoid refactoring our code structure to accommodate TS: we should try to make TS work with what we have.
Thanks, I was looking for that issue but I couldn't find it 😅 That's why the original title of this PR was "Don't use |
"composite": true in tsconfig.json"composite": true in tsc (until it supports cycles)
75b7fe5 to
c7c0b2b
Compare
Currently TSC doesn't support composite projects with circular references. This is a problem for us, since (especially when considering peer dependencies) we have many of them.
We could choose to ignore
peerDependencieswhen computing project references, but that ends up withimport x from "@babel/core"being typed asanyin packages with a peerDep on@babel/core. If we just stop using"composite": true, we don't have this problem because TS will load everything at the same time.This has some overhead when running
make tscheck, editing some files in a single package, and then runningmake tscheckagain: TS will have to check the whole monorepo rather than just the compiled files.However, almost any editor now has a native TS integration, so
make tscheckis something we only run once before committing.cc @zxbodya @JLHwung