993342 - [marionette-js-logger] Support multiple listeners by emitting events in addition to backwards-compatible handleMessage method

Status: RESOLVED FIXED

(Testing Graveyard :: JSMarionette, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

marionette-js-logger only supports a single consumer; the last code to clobber Server.handleMessage is the lucky winner of all of the log messages.  I think it would be good if it also was an EventEmitter so that if there are multiple consumers they can all get the logs.

My specific use-case is a wrapper that records a video of the app while also logging everything from the console at the same time.  Then we do a popcorn-project type thing where the log entries scroll as you watch the video and you can scrub video positions by clicking on log entries.  This optional thinger should not interfere with other explicit uses of the log messages by a test.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Attachment: make Server an EventEmitter

Unfortunately the prototype idiom had to be changed to use the standard EventEmitter inheritance idiom, so the diff looks a little more extensive than it is unless you tell it to ignore whitespace.  Convenience link for that:
https://github.com/mozilla-b2g/marionette-js-logger/pull/7/files?w=1

Comment 2

[Gareth Aye [:gaye] (back from PTO)]

Can't we keep things the same and do |Server.prototype = { __proto__: EventEmitter.prototype, ... };| ?

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Per https://groups.google.com/d/msg/mozilla.dev.platform/Yqw7rVs7pn4/XliS0NYaav8J apparently using __proto__ in the object initializer is not actually a great idea.  That is, of course, for SpiderMonkey, and V8 may behave differently/be fancier, but since the util.inherits thing is the node idiom anyways, it seemed like a better way to go.  I don't strongly care, I just in general feel bad for having suggested/propagated the __proto__ idiom when it turns out to be a bad idea.  (In my defense, I got some misleading info last time the __proto__ thing came up.)

Comment 4

[Gareth Aye [:gaye] (back from PTO)]

Comment on attachment 8403219 [details] [review]
make Server an EventEmitter

r=me once we get tests passing (see https://travis-ci.org/mozilla-b2g/marionette-js-logger/jobs/22521486). Thanks!

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Test-failure-wise, I did re-run the failing test and it turned green, but since intermittents are bad, I investigated.

As one of the older marionette js plugins, this had a lot of out-dated deps in package.json.  I've brought it up-to-speed and followed the lead of marionette-helper in terms of updating .travis.yml and the Makefile.  The mozilla-download usage is taken from gaia-email-libs-and-more.

I think this cleanup made it happier.  At least, I got a full green run on the first try and I re-ran the thing again and got a full green run after that.  So I think we're happy.


I think this is ready to merge and publish but:
- I don't have publish rights on npm
- the current version says it is version 0.1.2-non-native.  I'm unsure how the suffix gets there.  I might be inclined to leave the final merge to whoever does the publishing since they might need to make some other tweak?

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

landed as unpublished 0.13:
https://github.com/mozilla-b2g/marionette-js-logger/pull/7
https://github.com/mozilla-b2g/marionette-js-logger/commit/aba6b821a6c82fc615311a468bf6d97eb532e0f3

We won't be publishing this because we need to alter the logger to be synchronous-friendly which will rev it to 0.2.0.