doc, test: Symbols as event names

[bengl]

As a result of the new support for Symbols in V8, along with the implementation of EventEmitter in Node, it turns out that Symbols are usable as event names. This adds the possibility of Symbol event names to the docs, and adds an EventEmitter test specific to Symbols so as to ensure that this doesn't break due to any potential future alternative implementations.

[JungMinu]

CI:https://ci.nodejs.org/job/node-test-pull-request/924/

[cjihrig]

You can wrap this in common.mustCall(), then you don't need didRun.

[cjihrig]

LGTM with a few comments.

[Fishrock123]

Tests aren't happy:

not ok 230 test-event-emitter-symbols.js
# module.js:329
# throw err;
# ^
# # Error: Cannot find module 'common'
# at Function.Module._resolveFilename (module.js:327:15)
# at Function.Module._load (module.js:278:25)
# at Module.require (module.js:355:17)
# at require (internal/module.js:13:17)
# at Object.<anonymous> (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd102-64/test/parallel/test-event-emitter-symbols.js:3:1)
# at Module._compile (module.js:399:26)
# at Object.Module._extensions..js (module.js:406:10)
# at Module.load (module.js:345:32)
# at Function.Module._load (module.js:302:12)
# at Function.Module.runMain (module.js:431:10)

@bengl could you please be sure to run ./configure && make -j$(getconf _NPROCESSORS_ONLN) (make -j<nuber of processor threads>)

[Fishrock123]

I think this needs to be '../common'

[cjihrig]

This should be require('../common');

[bengl]

Fixed the common require, and used its mustCall to check that the listener is called. Also added an assertion as per @cjihrig's comments.

[jasnell]

Hmm.. is this actually something we want to promote? I know it works but should it. /cc @nodejs/ctc

[trevnorris]

It's not documented, but you can emit on objects as well.

constEE=require('events');conste=newEE();constb={foo: 42};e.on(b,function(){console.log('hi');});e.emit(b);

[bengl]

@trevnorris Sort of. Because b coerces to [object Object], emitting on any object will fire the same event as b.

constEE=require('events');conste=newEE();e.on({foo: 42},function(){console.log('hi');});e.emit({bar: 43});// hie.emit({baz: 44});// hi

Symbols don't get coerced for property identifiers though, so they're distinct here I think.

constEE=require('events');conste=newEE();consta=Symbol('foo');e.on(a,function(){console.log('hi');});e.emit(Symbol('foo'));// no hi

[trevnorris]

hah. nice. never bothered to test that thoroughly.

[bengl]

Perhaps "any valid property key" is the most correct thing to put here? That encompasses everything that's actually supported and doesn't encourage or discourage the use of Symbols.

[jasnell]

That may be better. I'm wondering too if we shouldn't simply throw if it's anything other than a "valid property key". Passing in an object simply doesn't make sense. Either way tho, this LGTM. Thanks!

[Qard]

👍 I like the idea of promoting symbol keys because it allows for private events on emitters.

[trevnorris]

@Qard Not really "private". Can still be just as easily accessed via Object.getOwnPropertySymbols(e._events). But at least there won't be any accidental name collisions.

[Qard]

Yeah, private-ish. There's always some escape hatch, isn't there? 😉

[bengl]

Alright I replaced "any string or symbol" with "any valid property key".

[jasnell]

LGTM

[jasnell]

@cjihrig .. does this still LGTY?

[cjihrig]

Yes, but I'd still like to see the additional assertion that I originally requested.

[jasnell]

@bengl ... can you take another look at @cjihrig's request. once that's updated I can get this landed

[bengl]

@cjihrig@jasnell OK, I think I've addressed that now.

[cjihrig]

Yes, thank you! LGTM

[jasnell]

Landed in 7a51878