-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix(core): improve buildExplicitTypeScriptDependnecies performance #33963
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,34 +56,29 @@ export function buildExplicitTypeScriptDependencies( | |
| '.mjs', | ||
| '.cjs', | ||
| '.cts', | ||
| '.vue', | ||
| ]; | ||
|
|
||
| // TODO: This can be removed when vue is stable | ||
| if (isVuePluginInstalled()) { | ||
| moduleExtensions.push('.vue'); | ||
| } | ||
|
|
||
| for (const [project, fileData] of Object.entries( | ||
| ctx.filesToProcess.projectFileMap | ||
| )) { | ||
| Object.keys(ctx.filesToProcess.projectFileMap).forEach((project) => { | ||
| filesToProcess[project] ??= []; | ||
| for (const { file } of fileData) { | ||
| if (moduleExtensions.some((ext) => file.endsWith(ext))) { | ||
| filesToProcess[project].push(join(workspaceRoot, file)); | ||
| ctx.filesToProcess.projectFileMap[project].forEach((fileData) => { | ||
| if (moduleExtensions.some((ext) => fileData.file.endsWith(ext))) { | ||
| filesToProcess[project].push(join(workspaceRoot, fileData.file)); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| const { findImports } = require('../../../../native'); | ||
|
|
||
| // TODO(jon): check why this function is very slow and whether we it can be optimized | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would be better for @FrozenPandaz
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All files get processed in parallel... it goes as fast as the largest file which is the slowest to process. IF this is taking long it likely mans that there is a considerably large typescript file. You can see which one it is with |
||
| const imports = findImports(filesToProcess); | ||
|
|
||
| for (const { | ||
| sourceProject, | ||
| file, | ||
| staticImportExpressions, | ||
| dynamicImportExpressions, | ||
| } of imports) { | ||
| for (let i = 0; i < imports.length; i++) { | ||
| const { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this actually yield any performance benefit?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does, however, in this location it's not large impact so if you don't like this old-school syntax we can revert this block. |
||
| sourceProject, | ||
| file, | ||
| staticImportExpressions, | ||
| dynamicImportExpressions, | ||
| } = imports[i]; | ||
| const normalizedFilePath = normalizePath(relative(workspaceRoot, file)); | ||
|
|
||
| for (const importExpr of staticImportExpressions) { | ||
|
|
@@ -131,13 +126,3 @@ export function buildExplicitTypeScriptDependencies( | |
|
|
||
| return res; | ||
| } | ||
|
|
||
| function isVuePluginInstalled() { | ||
| try { | ||
| // nx-ignore-next-line | ||
| require.resolve('@nx/vue'); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,13 +22,15 @@ import { | |
| getRootTsConfigFileName, | ||
| resolveModuleByImport, | ||
| } from '../../utils/typescript'; | ||
| import { existsSync } from 'node:fs'; | ||
|
|
||
| /** | ||
| * The key is a combination of the package name and the workspace relative directory | ||
| * containing the file importing it e.g. `lodash__packages/my-lib`, the value is the | ||
| * resolved external node name from the project graph. | ||
| */ | ||
| type NpmResolutionCache = Map<string, string | null>; | ||
| type PackageJsonResolutionCache = Map<string, PackageJson | null>; | ||
|
|
||
| type PathPattern = { | ||
| pattern: string; | ||
|
|
@@ -44,6 +46,7 @@ type ParsedPatterns = { | |
| * Use a shared cache to avoid repeated npm package resolution work within the TargetProjectLocator. | ||
| */ | ||
| const defaultNpmResolutionCache: NpmResolutionCache = new Map(); | ||
| const defaultPackageJsonResolutionCache: PackageJsonResolutionCache = new Map(); | ||
|
|
||
| const experimentalNodeModules = new Set(['node:sqlite']); | ||
|
|
||
|
|
@@ -71,7 +74,8 @@ export class TargetProjectLocator { | |
| string, | ||
| ProjectGraphExternalNode | ||
| > = {}, | ||
| private readonly npmResolutionCache: NpmResolutionCache = defaultNpmResolutionCache | ||
| private readonly npmResolutionCache: NpmResolutionCache = defaultNpmResolutionCache, | ||
| private readonly packageJsonResolutionCache: PackageJsonResolutionCache = defaultPackageJsonResolutionCache | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did not review too carefully.. but just to sanity check me... This is OK for the daemon because everytime we calculate dependencies, we create a brand new project locator with no cache right? So this cache will not remain between different graph calculations?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, exactly. The cache is reused only during the single CLI command/daemon OP lifetime. |
||
| ) { | ||
| /** | ||
| * Only the npm external nodes should be included. | ||
|
|
@@ -211,7 +215,8 @@ export class TargetProjectLocator { | |
| // package.json refers to an external package, we do not match against the version found in there, we instead try and resolve the relevant package how node would | ||
| const externalPackageJson = this.readPackageJson( | ||
| packageName, | ||
| fullDirPath | ||
| fullDirPath, | ||
| workspaceRoot | ||
| ); | ||
| // The external package.json path might be not be resolvable, e.g. if a reference has been added to a project package.json, but the install command has not been run yet. | ||
| if (!externalPackageJson) { | ||
|
|
@@ -533,18 +538,26 @@ export class TargetProjectLocator { | |
| */ | ||
| private readPackageJson( | ||
| packageName: string, | ||
| relativeToDir: string | ||
| relativeToDir: string, | ||
| workspaceRoot: string | ||
| ): PackageJson | null { | ||
| // The package.json is directly resolvable | ||
| const packageJsonPath = resolveRelativeToDir( | ||
| join(packageName, 'package.json'), | ||
| relativeToDir | ||
| ); | ||
| if (packageJsonPath) { | ||
| if (this.packageJsonResolutionCache.has(packageJsonPath)) { | ||
| return this.packageJsonResolutionCache.get(packageJsonPath); | ||
| } | ||
| const parsedPackageJson = readJsonFile(packageJsonPath); | ||
|
|
||
| if (parsedPackageJson.name && parsedPackageJson.version) { | ||
| this.packageJsonResolutionCache.set(packageJsonPath, parsedPackageJson); | ||
| return parsedPackageJson; | ||
| } else { | ||
| this.packageJsonResolutionCache.set(packageJsonPath, null); | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -556,14 +569,29 @@ export class TargetProjectLocator { | |
|
|
||
| while (dir !== dirname(dir)) { | ||
| const packageJsonPath = join(dir, 'package.json'); | ||
| try { | ||
| const parsedPackageJson = readJsonFile(packageJsonPath); | ||
| // Ensure the package.json contains the "name" and "version" fields | ||
| if (parsedPackageJson.name && parsedPackageJson.version) { | ||
| return parsedPackageJson; | ||
| if (this.packageJsonResolutionCache.has(packageJsonPath)) { | ||
| return this.packageJsonResolutionCache.get(packageJsonPath); | ||
| } | ||
| if (existsSync(packageJsonPath)) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should get rid of this existsSync.. it will throw an error if it doesn't exist which is already handled
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's more common for the file to not exist (falling through down to the root package.json). The |
||
| try { | ||
| const parsedPackageJson = readJsonFile(packageJsonPath); | ||
| // Ensure the package.json contains the "name" and "version" fields | ||
| if (parsedPackageJson.name && parsedPackageJson.version) { | ||
| this.packageJsonResolutionCache.set( | ||
| packageJsonPath, | ||
| parsedPackageJson | ||
| ); | ||
| return parsedPackageJson; | ||
| } else { | ||
| this.packageJsonResolutionCache.set(packageJsonPath, null); | ||
| return null; | ||
| } | ||
| } catch { | ||
| // Package.json is invalid, keep traversing | ||
| } | ||
| } catch { | ||
| // Package.json doesn't exist, keep traversing | ||
| } | ||
| if (dir === workspaceRoot) { | ||
| return null; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So before.. we were able to.. resolve outside of the workspace root? Have we considered if this actually helps when the workspaceRoot is not the same as the node_modules root? WHat if someone has like.. repo-root/package.json If this doesn't yield a major perf behefit, I would not make htis change for now
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can test on the customer repo if reverting this change impacts performance. |
||
| } | ||
| dir = dirname(dir); | ||
meeroslav marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
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.
Does this actually yield a performance benefit?
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.
Yes, I don't have the numbers anymore, but it shaved off 100+ms IIRC.