-
Notifications
You must be signed in to change notification settings - Fork 182
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
Report heap usage status with sonar.javascript.node.debugMemory
#4294
Changes from 2 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 |
---|---|---|
|
@@ -151,6 +151,7 @@ | |
"bin/" | ||
], | ||
"_moduleAliases": { | ||
"@sonar/bridge": "lib/bridge/src", | ||
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 had to do this to be able to |
||
"@sonar/css": "lib/css/src", | ||
"@sonar/html": "lib/html/src", | ||
"@sonar/jsts": "lib/jsts/src", | ||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -35,6 +35,7 @@ const { analyzeCSS } = require('@sonar/css'); | |||||||
const { analyzeHTML } = require('@sonar/html'); | ||||||||
const { analyzeYAML } = require('@sonar/yaml'); | ||||||||
const { APIError, ErrorCode } = require('@sonar/shared/errors'); | ||||||||
const { logHeapStatistics } = require('@sonar/bridge/memory'); | ||||||||
|
||||||||
/** | ||||||||
* Delegate the handling of an HTTP request to a worker thread | ||||||||
|
@@ -109,6 +110,7 @@ if (parentPort) { | |||||||
|
||||||||
case 'on-create-program': { | ||||||||
const { tsConfig } = data; | ||||||||
logHeapStatistics(); | ||||||||
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. Just to be sure that it's intentional, logs will only be printed when analyzing with programs, not with watch programs, right? We might face a weird situation where SonarLint users see the log about out-of-memory suggesting to enable the property Maybe the following line should be executed conditionally, when SonarJS/packages/bridge/src/memory.ts Lines 70 to 72 in 0ef5749
What do you think? 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, you are right, however, it's quite a corner case, I would not overcomplicate this 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. 👍 |
||||||||
const { programId, files, projectReferences, missingTsConfig } = | ||||||||
createAndSaveProgram(tsConfig); | ||||||||
parentThread.postMessage({ | ||||||||
|
@@ -128,6 +130,7 @@ if (parentPort) { | |||||||
case 'on-delete-program': { | ||||||||
const { programId } = data; | ||||||||
deleteProgram(programId); | ||||||||
logHeapStatistics(); | ||||||||
parentThread.postMessage({ type: 'success', result: 'OK!' }); | ||||||||
break; | ||||||||
} | ||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -146,7 +146,7 @@ describe('server', () => { | |
await new Promise(r => setTimeout(r, 600)); | ||
expect(server.listening).toBeFalsy(); | ||
|
||
expect(console.log).toHaveBeenCalledWith('DEBUG The bridge server shutted down'); | ||
expect(console.log).toHaveBeenCalledWith('DEBUG The bridge server shut down'); | ||
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. Thanks! |
||
}); | ||
}); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,7 @@ export interface Context { | |
workDir: string; | ||
shouldUseTypeScriptParserForJS: boolean; | ||
sonarlint: boolean; | ||
debugMemory?: boolean; | ||
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. Did you mark the field as optional because some unit tests would fail otherwise? 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. exactly, it's created in many places. 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. 👍 |
||
bundles: string[]; | ||
} | ||
|
||
|
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.
Maybe we could get rid of the if-statement and replace the bundles-related lines with: