-
Notifications
You must be signed in to change notification settings - Fork 469
Bin clean up #6401
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
Bin clean up #6401
Conversation
@@ -20,23 +20,24 @@ var LAST_FIRED_EVENT = 0; | |||
/** | |||
* @type {[string,string][]} | |||
*/ | |||
var reasons_to_rebuild = [["proj", "started"]]; | |||
var reasonsToRebuild = [["proj", "started"]]; |
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.
Use camel case as a conventional style in the JS ecosystem.
|
||
var LAST_SUCCESS_BUILD_STAMP = 0; | ||
var cwd = process.cwd(); | ||
var lockFileName = path.join(cwd, ".bsb.lock"); | ||
process.env.BSB_PROJECT_ROOT = cwd; | ||
// console.log('BSB_PROJECT_ROOT:', process.env.BSB_PROJECT_ROOT) |
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.
Removed the redundant log
var error_is_tty = process.stderr.isTTY; | ||
var std_is_tty = process.stdout.isTTY; |
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.
Moved to the top, so they become available in the notifyClients
function. There used to be an error.
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 these not also be changed to camel case if we're trying to make things in one style? I don't mind either way, but asking just in case.
|
||
// If the project uses gentype and uses custom file extension | ||
// via generatedFileExtension, ignore them in watch mode | ||
var bsConfigFile = path.join(cwd, bsconfig); | ||
var genTypeFileExtension = undefined; | ||
|
||
var genTypeFileExtension = ".gen.tsx"; |
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.
Prefilled the default value, so we do one check less in the checkIsRebuildReason
function (previously valid_event
)
/** | ||
* @returns {string} | ||
*/ | ||
function getDateAsString() { |
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 wasn't used
* @param {boolean} withWebSocket | ||
*/ | ||
function in_watch_mode(useWebSocket) { | ||
if (useWebSocket) { | ||
function startWatchMode(withWebSocket) { |
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.
Not the best name, but better than before
if (code !== 0) { | ||
console.log(log); |
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 might make sense to display build time even when it failed, but I've decided to keep it as it is.
} else { | ||
console.log(">>>> Finish compiling(exit: " + code + ")"); | ||
} | ||
console.log(log, Math.floor(updateFinishTime() / 1e6), "mseconds"); |
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.
Also, I fixed the build time display for non-TTY mode.
if (code === 0) { | ||
LAST_SUCCESS_BUILD_STAMP = +new Date(); | ||
LAST_SUCCESS_BUILD_STAMP = Date.now(); |
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.
Date.now()
is 15% faster than +new Date()
. A microptimisation, but still 😊
LAST_FIRED_EVENT = event_time; | ||
reasons_to_rebuild.push([event, reason]); | ||
LAST_FIRED_EVENT = eventTime; | ||
reasonsToRebuild.push([event, reason || ""]); |
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've fixed the type, so the fallback became needed.
Sweet. Are there any specific node requirements for this? CC @zth |
There shouldn't be. But I'll test with NodeJs 10 just in case. |
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.
Looks good! I don't think it requires any new Node capabilities but yeah it's good if you test with Node v10 @DZakh
var error_is_tty = process.stderr.isTTY; | ||
var std_is_tty = process.stdout.isTTY; |
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 these not also be changed to camel case if we're trying to make things in one style? I don't mind either way, but asking just in case.
I've tested with node V10. It works.
I couldn't come up with a good name for them, so decided to leave them for the next PR 😅 |
I want to do a few fixes in the file, so I've decided to start with a lil clean up.
++ Display build time in the non-TTY mode