-
Notifications
You must be signed in to change notification settings - Fork 133
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
Site.js: remove site generation profiling #748
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.
Hi Hiroki, thank you for your contribution!
src/Site.js
Outdated
.finally(() => { | ||
const endTime = new Date(); | ||
const totalBuildTime = endTime - startTime; | ||
logger.debug(`Total generation time: ${totalBuildTime}ms`); |
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.
Let's:
- rename this variable to
totalGenerationTime
. - use seconds, to be consistent with the existing
totalBuildTime
. - remove the
profile
export in logger.js.
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.
Should we downgrade the profile logging in logger.js#profile()
instead, like so:
logger.profile('test', { level: 'debug' });
Otherwise, if we are not planning to do any extensive profiling in the future, there's not really a reason for this portion of the code to stay.
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 seems necessary to upgrade the version of logger (winston) from 2 to 3 in order to execute that code. logger.profile('test', { level: 'debug' });
https://github.com/winstonjs/winston/blob/master/UPGRADE-3.0.md
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.
Let's just get rid of this part of the code then.
Your first post and PR title will need to be updated, the PR title can be |
Title updated |
Thank you for your reviews. 🙇 |
What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [X] Bug fix
Fixes #738
What changes did you make? (Give an overview)
Get rid of the logger.profile()
use Date() and show generate time on debug level