-
-
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
update tsconfig generation to reference dependencies used in source code #12941
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/43488/ |
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 0b4e16e:
|
scripts/generators/tsconfig.js
Outdated
@@ -2,6 +2,10 @@ import path from "path"; | |||
import fs from "fs"; | |||
import { createRequire } from "module"; | |||
import { fileURLToPath } from "url"; | |||
import globby from "globby"; | |||
import * as babel from "@babel/core"; | |||
import traverseModule from "@babel/traverse"; |
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.
when running it locally instead of default export - whole module is imported here
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 expected, because @babel/traverse
is not a native module but it's compiled to CJS.
scripts/generators/tsconfig.js
Outdated
const ast = babel.parseSync(source, { | ||
ast: true, | ||
filename, | ||
}); | ||
|
||
traverse(ast, { | ||
ImportDeclaration(path) { | ||
const importSource = path.node.source.value; | ||
if (importSource.startsWith("@babel/")) { | ||
result.add(importSource); | ||
} | ||
}, | ||
}); | ||
} |
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 wonder if we could speed this up with
for (const [importSource] of source.matchAll(/(?<=from\s*")@babel\/[^"/]+/g)) {
result.add(importSource);
}
this doesn't work for generic JS, but it should be ok for our own source files.
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.
that actually not only faster - it also covers additional cases when when having export {} from ""
statement
updated the PR
I have no idea why CI is failing, I'll investigate. |
I cannot reproduce the failure locally, and GH doesn't let me clean the actions cache. Could you try rebasing this on |
in some cases there are devDependecnies used for type-checking, this change adds references to them when generationg tsconfig
0480e16
to
afd5c7a
Compare
rebased it |
If CI passes you can revert 5648962 |
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.
Approving, pending the revert of 5648962.
Note for other reviewers: the reason this is needed is that relying on devDependencies
introduces many circular dependencies between packages, and TS doesn't handle them well. What this PR matches is a subset of actual deps/devDeps, because many devDeps are only used in tests.
This reverts commit 5648962.
globby
In some cases there are
devDependecnies
used for type-checking(like on@babel/traverse
to getNodePath
type).This changes how tsconfig.json files are generated to include references, to babel packages imported from package source code.