Skip to content
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

Merged
merged 1 commit into from
May 30, 2018
Merged

refactor(launcher): Update launcher to ES6 #3014

merged 1 commit into from
May 30, 2018

Conversation

lusarz
Copy link
Contributor

@lusarz lusarz commented May 24, 2018

Within this pull request I've used a few ES6 features in lib/launcher.js file to improve it's readability.

lib/launcher.js Outdated
return !browser.isCaptured()
})
this.areAllCaptured = () => {
return this._browsers.every((browser) => browser.isCaptured())
Copy link
Contributor

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())
?

Copy link
Contributor Author

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()
Copy link
Contributor

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.

Copy link
Contributor Author

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) {
Copy link
Collaborator

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?

Copy link
Contributor

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).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okey, I see.

@johnjbarton
Copy link
Contributor

This PR now conflicts with master.

@lusarz
Copy link
Contributor Author

lusarz commented May 30, 2018

Conflicts with master fixed

@johnjbarton johnjbarton merged commit f5cda4d into karma-runner:master May 30, 2018
@lusarz lusarz deleted the refactor-launcher branch June 4, 2018 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants