-
Notifications
You must be signed in to change notification settings - Fork 648
Set maxNodeModuleJsDepth in JS-containing projects #1213
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: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR adds support for setting a maximum depth for Node.js modules in projects that contain JavaScript files. Key changes include:
- Introducing a new field (rootJSFileCount) to track the count of JS files.
- Updating project configuration in updateProgram() to set a default MaxNodeModuleJsDepth when JS files are present.
- Refactoring root file management by replacing direct map access with helper methods that update the JS file count.
@@ -561,6 +562,12 @@ func (p *Project) updateProgram() bool { | |||
} else { | |||
rootFileNames := p.GetRootFileNames() | |||
compilerOptions := p.compilerOptions | |||
|
|||
if compilerOptions.MaxNodeModuleJsDepth == nil && p.rootJSFileCount > 0 { | |||
compilerOptions = compilerOptions.Clone() |
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 is method GetCompilerOptions - do you need to modify that instead and scan through other usage of p.compilerOptions ?
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 method is actually unused except for the API package, so I opted to not modify that. I could theoretically do that, though, yes.
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.
Tried, but the concurrency story on this is pretty weird. I'd rather just leave this here for now and fix it if we need this option set for something else eventually. It'll probably get easier with the snapshot refactor.
Could use a test, but this looks correct to me in theory.
Fixes #1209