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
fix(file-reporter): output logs incrementally #5011
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 |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@pnpm/file-reporter": minor | ||
--- | ||
|
||
Logs will be added as they are created instead of all at once at the end. Prevents memory exceeded errors if there are too many logs. |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -4,49 +4,40 @@ import fs from 'graceful-fs' | |||||
const LOG_FILENAME = 'node_modules/.pnpm-debug.log' | ||||||
|
||||||
export default function (streamParser: Object) { | ||||||
const logs: Object[] = [] | ||||||
let logNum = 0 | ||||||
|
||||||
// Clean up previous log files | ||||||
if (fs.existsSync(LOG_FILENAME)) fs.unlinkSync(LOG_FILENAME) | ||||||
if (fs.existsSync(path.basename(LOG_FILENAME))) fs.unlinkSync(path.basename(LOG_FILENAME)) | ||||||
|
||||||
streamParser['on']('data', function (logObj: Object) { | ||||||
if (isUsefulLog(logObj)) { | ||||||
logs.push(logObj) | ||||||
if (isUsefulLog(logObj) && global['writeDebugLogFile'] !== false) { | ||||||
const msgobj = getMessageObj(logObj) | ||||||
const prettyLogs = prettify(msgobj) | ||||||
const jsonLogs = JSON.stringify(prettyLogs, null, 2) + '\n' | ||||||
const dest = fs.existsSync(path.dirname(LOG_FILENAME)) ? LOG_FILENAME : path.basename(LOG_FILENAME) | ||||||
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 don't think it is optimal to check the existence of the file hundreds of times. 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. also, why are there two possible locations for the logs? |
||||||
fs.appendFileSync(dest, jsonLogs) | ||||||
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. is this the best way to do this? Or would it be faster to open a write stream? That would keep up the connection to the file, I guess. 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 can probably look at how winston does it |
||||||
logNum++ | ||||||
} | ||||||
}) | ||||||
|
||||||
process.on('exit', (code: number) => { | ||||||
if (code === 0 || global['writeDebugLogFile'] === false) { | ||||||
// it might not exist, so it is OK if it fails | ||||||
try { | ||||||
fs.unlinkSync(LOG_FILENAME) | ||||||
} catch (err) {} | ||||||
return | ||||||
if (fs.existsSync(LOG_FILENAME)) fs.unlinkSync(LOG_FILENAME) | ||||||
if (fs.existsSync(path.basename(LOG_FILENAME))) fs.unlinkSync(path.basename(LOG_FILENAME)) | ||||||
} | ||||||
|
||||||
const prettyLogs = getPrettyLogs() | ||||||
const jsonLogs = JSON.stringify(prettyLogs, null, 2) | ||||||
// Don't create a node_modules directory if it does not exist | ||||||
const dest = fs.existsSync(path.dirname(LOG_FILENAME)) | ||||||
? LOG_FILENAME | ||||||
: path.basename(LOG_FILENAME) | ||||||
fs.writeFileSync(dest, jsonLogs, 'utf-8') | ||||||
}) | ||||||
|
||||||
function getPrettyLogs () { | ||||||
const prettyLogs = {} | ||||||
logs.forEach((logObj, i) => { | ||||||
// eslint-disable-next-line | ||||||
const key = `${i} ${logObj['level']} ${logObj['name']}` | ||||||
const msgobj = getMessageObj(logObj) | ||||||
prettyLogs[key] = prettify(msgobj) | ||||||
}) | ||||||
return prettyLogs | ||||||
} | ||||||
|
||||||
function getMessageObj (logobj: Object): Object { | ||||||
const msgobj = {} | ||||||
const msgobj: { [key: string]: string } = {} | ||||||
for (const key in logobj) { | ||||||
if (['time', 'hostname', 'pid', 'level', 'name'].includes(key)) continue | ||||||
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.
Suggested change
|
||||||
msgobj[key] = logobj[key] | ||||||
} | ||||||
const logLevel: string = logobj['level'] | ||||||
const logName: string = logobj['name'] | ||||||
msgobj.key = `${logNum} ${logLevel} ${logName}` | ||||||
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 field can be removed. |
||||||
return msgobj | ||||||
} | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,9 @@ | ||
{ | ||
"0 info pnpm": "foo", | ||
"1 warn pnpm": { | ||
"foo": 1, | ||
"bar": 2 | ||
} | ||
} | ||
"message": "foo", | ||
"key": "0 info pnpm" | ||
} | ||
{ | ||
"foo": 1, | ||
"bar": 2, | ||
"key": "1 warn pnpm" | ||
} |
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 think if global['writeDebugLogFile'] is false, we can early return from the outer function at line 7