Skip to content

Commit

Permalink
refactor(events): refactor karma EventEmitter (#3012)
Browse files Browse the repository at this point in the history
  • Loading branch information
lusarz authored and johnjbarton committed May 23, 2018
1 parent 667b47e commit 8197408
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 61 deletions.
85 changes: 32 additions & 53 deletions lib/events.js
Original file line number Diff line number Diff line change
@@ -1,75 +1,58 @@
var events = require('events')
var util = require('util')
'use strict'

var helper = require('./helper')
const EventEmitter = require('events').EventEmitter
const helper = require('./helper')

var bindAllEvents = function (object, context) {
context = context || this
function bufferEvents (emitter, eventsToBuffer) {
const listeners = []
const eventsToReply = []

var bindMethod = function (method) {
context.on(helper.camelToSnake(method.substr(2)), function () {
var args = Array.prototype.slice.call(arguments, 0)
args.push(context)
object[method].apply(object, args)
})
function genericListener () {
eventsToReply.push(Array.from(arguments))
}

for (var method in object) {
if (helper.isFunction(object[method]) && method.substr(0, 2) === 'on') {
bindMethod(method)
}
}
}

var bufferEvents = function (emitter, eventsToBuffer) {
var listeners = []
var eventsToReply = []
var genericListener = function () {
eventsToReply.push(Array.prototype.slice.call(arguments))
}

eventsToBuffer.forEach(function (eventName) {
var listener = genericListener.bind(null, eventName)
eventsToBuffer.forEach((eventName) => {
const listener = genericListener.bind(null, eventName)
listeners.push(listener)
emitter.on(eventName, listener)
})

return function () {
if (!eventsToReply) {
return
}

// remove all buffering listeners
listeners.forEach(function (listener, i) {
listeners.forEach((listener, i) => {
emitter.removeListener(eventsToBuffer[i], listener)
})

// reply
eventsToReply.forEach(function (args) {
events.EventEmitter.prototype.emit.apply(emitter, args)
eventsToReply.forEach((args) => {
EventEmitter.prototype.emit.apply(emitter, args)
})

// free-up
listeners = eventsToReply = null
listeners.length = 0
eventsToReply.length = 0
}
}

// TODO(vojta): log.debug all events
var EventEmitter = function () {
this.bind = bindAllEvents
class KarmaEventEmitter extends EventEmitter {
bind (object) {
Object.keys(object).forEach((method) => {

This comment has been minimized.

Copy link
@johnjbarton

johnjbarton Jun 20, 2018

Contributor

Unfortunately this refactoring was not correct.

Object.keys() provides the enumerable own properties of an object.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/keys

for...in iterates the enumerable properties including the prototype properties
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in

This difference shows up when an event emitter (eg a reporter) is written using es6 class or older equivalent approaches. The super old karma code tends to write objects with methods as own properties so the two solutions do not differ.

For the purpose of the EventEmitter we want for..in so we bind the class methods.

(To be sure I really dislike this way-too-tricky bind-with-name-hacking code).

This comment has been minimized.

Copy link
@devoto13

devoto13 Jun 20, 2018

Collaborator

@johnjbarton Just want to note that neither of these approaches will work with real ES2015 classes, because real ES2015 classes define methods as not enumerable properties.

This is one of the reasons, why I was refactoring bind into explicit subscriptions, when converting to real ES2015 classes. I think it would be good to refactor remaining usages, deprecate this method and eventually remove it.

This comment has been minimized.

Copy link
@johnjbarton

johnjbarton Jun 20, 2018

Contributor

Sure, but my goal now is to get the previous API working.

if (method.startsWith('on') && helper.isFunction(object[method])) {
this.on(helper.camelToSnake(method.substr(2)), function () {
object[method].apply(object, Array.from(arguments).concat(this))
})
}
})
}

this.emitAsync = function (name) {
emitAsync (name) {
// TODO(vojta): allow passing args
// TODO(vojta): ignore/throw if listener call done() multiple times
var pending = this.listeners(name).length
var deferred = helper.defer()
var done = function () {
let pending = this.listeners(name).length
const deferred = helper.defer()

this.emit(name, () => {
if (!--pending) {
deferred.resolve()
}
}

this.emit(name, done)
})

if (!pending) {
deferred.resolve()
Expand All @@ -79,9 +62,5 @@ var EventEmitter = function () {
}
}

util.inherits(EventEmitter, events.EventEmitter)

// PUBLISH
exports.EventEmitter = EventEmitter
exports.bindAll = bindAllEvents
exports.EventEmitter = KarmaEventEmitter
exports.bufferEvents = bufferEvents
4 changes: 3 additions & 1 deletion lib/launchers/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ var BaseLauncher = function (id, emitter) {
Object.keys(EventEmitter.prototype).forEach(function (method) {
this[method] = EventEmitter.prototype[method]
}, this)
KarmaEventEmitter.call(this)

this.bind = KarmaEventEmitter.prototype.bind.bind(this)
this.emitAsync = KarmaEventEmitter.prototype.emitAsync.bind(this)

this.id = id
this.state = null
Expand Down
14 changes: 10 additions & 4 deletions lib/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ var FileList = require('./file-list')
var reporter = require('./reporter')
var helper = require('./helper')
var events = require('./events')
var EventEmitter = events.EventEmitter
var KarmaEventEmitter = events.EventEmitter
var EventEmitter = require('events').EventEmitter
var Executor = require('./executor')
var Browser = require('./browser')
var BrowserCollection = require('./browser_collection')
Expand All @@ -32,7 +33,7 @@ const karmaJsPath = path.join(__dirname, '/../static/karma.js')
const contextJsPath = path.join(__dirname, '/../static/context.js')

// Dynamic dependence on brwoserify
var browserify = null;
var browserify = null

/**
* Bundles a static resource using Browserify.
Expand Down Expand Up @@ -71,7 +72,12 @@ function createSocketIoServer (webServer, executor, config) {

// Constructor
var Server = function (cliOptions, done) {
EventEmitter.call(this)
Object.keys(EventEmitter.prototype).forEach(function (method) {
this[method] = EventEmitter.prototype[method]
}, this)

this.bind = KarmaEventEmitter.prototype.bind.bind(this)
this.emitAsync = KarmaEventEmitter.prototype.emitAsync.bind(this)

logger.setupFromConfig(cliOptions)

Expand Down Expand Up @@ -156,7 +162,7 @@ Server.prototype.refreshFiles = function () {
// ---------------

Server.prototype._start = function (config, launcher, preprocess, fileList,
capturedBrowsers, executor, done) {
capturedBrowsers, executor, done) {
var self = this
if (config.detached) {
this._detach(config, done)
Expand Down
6 changes: 3 additions & 3 deletions test/unit/events.spec.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
var e = require('../../lib/events')
const e = require('../../lib/events')

describe('events', () => {
var emitter
Expand Down Expand Up @@ -98,7 +98,7 @@ describe('events', () => {
it('should take emitter as second argument', () => {
var object = sinon.stub({onFoo: () => {}})

e.bindAll(object, emitter)
emitter.bind(object)
emitter.emit('foo')
emitter.emit('bar')

Expand All @@ -108,7 +108,7 @@ describe('events', () => {
it('should append "context" to event arguments', () => {
var object = sinon.stub({onFoo: () => {}})

e.bindAll(object, emitter)
emitter.bind(object)
emitter.emit('foo', 'event-argument')

expect(object.onFoo).to.have.been.calledWith('event-argument', emitter)
Expand Down

0 comments on commit 8197408

Please sign in to comment.