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

doc, test: Symbols as event names #4151

Closed
wants to merge 1 commit into from
Closed

Conversation

bengl
Copy link
Member

@bengl bengl commented Dec 4, 2015

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 JungMinu added doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. labels Dec 4, 2015
@mscdex mscdex added the events Issues and PRs related to the events subsystem / EventEmitter. label Dec 4, 2015
@JungMinu
Copy link
Member

JungMinu commented Dec 4, 2015

const foo = Symbol('foo');
let didRun = false;

function listener() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@cjihrig
Copy link
Contributor

cjihrig commented Dec 4, 2015

LGTM with a few comments.

@Fishrock123
Copy link
Contributor

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

@@ -0,0 +1,27 @@
'use strict';

require('common');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@bengl bengl force-pushed the symbolevents branch 2 times, most recently from 638e599 to f0f0484 Compare December 4, 2015 17:16
@bengl
Copy link
Member Author

bengl commented Dec 4, 2015

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

@@ -10,7 +10,8 @@ is opened. All objects which emit events are instances of `events.EventEmitter`.
You can access this module by doing: `require("events");`

Typically, event names are represented by a camel-cased string, however,
there aren't any strict restrictions on that, as any string will be accepted.
there aren't any strict restrictions on that, as any string or symbol will be
accepted.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

const EE = require('events');
const e = new EE();
const b = { foo: 42 };
e.on(b, function() { console.log('hi'); });
e.emit(b);

Copy link
Member Author

Choose a reason for hiding this comment

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

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

const EE = require('events');
const e = new EE();
e.on({ foo: 42 }, function() { console.log('hi'); });
e.emit({ bar: 43 }); // hi
e.emit({ baz: 44 }); // hi

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

const EE = require('events');
const e = new EE();
const a = Symbol('foo');
e.on(a, function() { console.log('hi'); });
e.emit(Symbol('foo')); // no hi

Copy link
Contributor

Choose a reason for hiding this comment

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

hah. nice. never bothered to test that thoroughly.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Qard commented Dec 8, 2015

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

@trevnorris
Copy link
Contributor

@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
Copy link
Member

Qard commented Dec 9, 2015

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

@bengl
Copy link
Member Author

bengl commented Dec 11, 2015

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

@jasnell
Copy link
Member

jasnell commented Dec 11, 2015

LGTM

@jasnell
Copy link
Member

jasnell commented Dec 14, 2015

@cjihrig .. does this still LGTY?

@cjihrig
Copy link
Contributor

cjihrig commented Dec 14, 2015

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

@jasnell
Copy link
Member

jasnell commented Dec 14, 2015

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

@bengl
Copy link
Member Author

bengl commented Dec 14, 2015

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

@cjihrig
Copy link
Contributor

cjihrig commented Dec 14, 2015

Yes, thank you! LGTM

jasnell pushed a commit that referenced this pull request Dec 14, 2015
* Document that Symbol can used as event names.
* Add test for using Symbol as event names

PR-URL: #4151
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Dec 14, 2015

Landed in 7a51878

@jasnell jasnell closed this Dec 14, 2015
cjihrig pushed a commit that referenced this pull request Dec 15, 2015
* Document that Symbol can used as event names.
* Add test for using Symbol as event names

PR-URL: #4151
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@rvagg rvagg mentioned this pull request Dec 17, 2015
MylesBorins pushed a commit that referenced this pull request Dec 29, 2015
* Document that Symbol can used as event names.
* Add test for using Symbol as event names

PR-URL: #4151
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
* Document that Symbol can used as event names.
* Add test for using Symbol as event names

PR-URL: #4151
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
* Document that Symbol can used as event names.
* Add test for using Symbol as event names

PR-URL: nodejs#4151
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. events Issues and PRs related to the events subsystem / EventEmitter. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants