-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
refactor(launcher): Update launcher to ES6 #3014
Conversation
lib/launcher.js
Outdated
return !browser.isCaptured() | ||
}) | ||
this.areAllCaptured = () => { | ||
return this._browsers.every((browser) => browser.isCaptured()) |
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.
Can we use an expression return:
() => this._browsers.every((browser) => browser.isCaptured())
?
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.
Good point, fixed!
lib/launcher.js
Outdated
} | ||
} | ||
|
||
this.launch = function (names, concurrency) { | ||
this.launch = (names, concurrency) => { | ||
lastStartTime = 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.
To be extra careful let's put this after new Jobs as before.
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.
Sure, fixed
@@ -25,34 +26,22 @@ var baseBrowserDecoratorFactory = function ( | |||
} | |||
} | |||
|
|||
var Launcher = function (server, emitter, injector) { | |||
function Launcher (server, emitter, injector) { |
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.
Why not update it to be real class
?
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.
(Actually I prefer small refactoring steps, so doing this next is my pref).
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.
Okey, I see.
This PR now conflicts with master. |
Conflicts with master fixed |
Within this pull request I've used a few ES6 features in
lib/launcher.js
file to improve it's readability.