Adding multiple listeners at once #15787

Giovarco · 2017-10-05T19:54:38Z

This pull request is all about solve this open issue / feature request: #15697 .

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

events

- Add onMultiple function

- Add unit tests

- Change name of the unit test

- Use symbol on the unit test

mscdex · 2017-10-05T20:15:45Z

With a more general function name like this, we may want to consider also adding support for multiple listeners as well.

Also supporting the same signature as .on() may be preferrable (a string and a function).

Type checking will also be needed no matter what.

Giovarco · 2017-10-05T20:32:38Z

@mscdex

With a more general function name like this, we may want to consider also adding support for multiple listeners as well.

Can you be more specific? Do you propose to add one event to multiple handlers beside multiple events to one handler?

Also supporting the same signature as .on() may be preferrable (a string and a function).

So, instead of .onMultiple(["A", "B"], handler), .onMultiple("A,B", handler)?

Type checking will also be needed no matter what.

No problem, easy peasy. But something strange I noticed is that no type checking is done in addListener(target, type, listener, prepend), am I wrong? Maybe it would be good to add it there instead of in .onMultiple().

mscdex · 2017-10-05T20:43:00Z

Can you be more specific? Do you propose to add one event to multiple handlers beside multiple events to one handler?

I meant being able to do any one of:

ee.onMultiple(['foo','bar','baz'], handler);

ee.onMultiple('foo', [handler1, handler2, handler3]);

// To clarify, this example would add each of the 3 handlers to each event passed
ee.onMultiple(['foo','bar','baz'], [handler1, handler2, handler3]);

As previously suggested in the original discussion on this, it might also be beneficial to support object variants, like:

ee.onMultiple({
  foo: handler1,
  bar: [handler2, handler3]
});

So, instead of .onMultiple(["A", "B"], handler), .onMultiple("A,B", handler)?

No, I mean:

ee.onMultiple('foo', handler);

to avoid users having to do their own checking to know whether they should call .onMultiple() or .on().

No problem, easy peasy. But something strange I noticed is that no type checking is done in addListener(target, type, listener, prepend), am I wrong? Maybe it would be good to add it there instead of in .onMultiple().

That's because the type checking for .on()/.prependListener() is done inside _addListener(), where the actual implementation is.

benjamingr · 2017-10-05T23:49:17Z

test/parallel/test-event-emitter-onMultiple.js

+    const emptyFunction = function() {};
+
+    // Test
+    await myEE.onMultiple(inputArray, emptyFunction);


Why is this awaited? I'm also not sure this actually tests the feature itself.

Why is this awaited?

Good point. I'll fix it. Thanks.

I'm also not sure this actually tests the feature itself.

To me, it seems like you're not conviced that using .eventNames() isn't the proper way to test the code. I mean, I could agree on some degree. How do you propose to change it?

bnoordhuis · 2017-10-06T06:59:47Z

make -j4 test (UNIX), or vcbuild test (Windows) passes

That seems implausible because make lint (which runs as part of make test) doesn't.

Giovarco · 2017-10-06T07:14:05Z

@bnoordhuis Sorry, I misinterpreted that point. One question: if I lint, will the indentation of my code be gone?

bnoordhuis · 2017-10-06T07:16:26Z

The linter only points out issues, it doesn't auto-fix them.

Giovarco · 2017-10-06T11:05:44Z

@bnoordhuis I tried to execute ./configure && make -j4 test. I got a huge wall of text that for me makes no sense. What does this mean?

(cut because too long)

bnoordhuis · 2017-10-06T11:07:44Z

Do you have spaces (Git projects) in the path? That won't work.

- Accept an object as input parameter now - Change unit tests accordingly

Giovarco · 2017-10-06T19:33:51Z

Ok now this syntax is used:

ee.onMultiple({
  foo: handler1,
  bar: [handler2, handler3]
});

I changed my unit test accordingly (now bad cases are handled yet). No type check is done since it is done in .on function. I am not sure if this is the proper way to write an unit test for an event emitter. I normally use Mocha.

P.S.: My test passes. Now I am running all the test suites

Giovarco · 2017-10-06T19:47:15Z

It seems like all tests passed, but in the end I get this error:

/usr/local/bin/python2.7 tools/test.py --mode=release -J \
        async-hooks \
        default \
        addons addons-napi \
        known_issues
=== release test-fs-copyfile ===
Path: parallel/test-fs-copyfile
assert.js:45
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 33188 === 33204
    at verify (/home/mario/Git_projects/node/node/test/parallel/test-fs-copyfile.js:17:10)
    at Object.<anonymous> (/home/mario/Git_projects/node/node/test/parallel/test-fs-copyfile.js:32:1)
    at Module._compile (module.js:600:30)
    at Object.Module._extensions..js (module.js:611:10)
    at Module.load (module.js:521:32)
    at tryModuleLoad (module.js:484:12)
    at Function.Module._load (module.js:476:3)
    at Function.Module.runMain (module.js:641:10)
    at startup (bootstrap_node.js:187:16)
    at bootstrap_node.js:605:3
Command: out/Release/node /home/mario/Git_projects/node/node/test/parallel/test-fs-copyfile.js
[04:26|% 100|+ 1971|-   1]: Done
Makefile:201: recipe for target 'test' failed
make: *** [test] Error 1

What does it mean?

mscdex · 2017-10-06T20:03:46Z

@Giovarco Pull in the latest changes from node master and the error should go away.

Giovarco · 2017-10-07T13:32:27Z

@mscdex I moved to local master and pulled from remote and nothing apparently happened. Now I stop here:

/usr/local/bin/python2.7 tools/test.py --mode=release -J \
        async-hooks \
        default \
        addons addons-napi \
        known_issues
=== release test-fs-copyfile ===
Path: parallel/test-fs-copyfile
assert.js:45
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 33188 === 33204
    at verify (/home/mario/Git_projects/node/node/test/parallel/test-fs-copyfile.js:17:10)
    at Object.<anonymous> (/home/mario/Git_projects/node/node/test/parallel/test-fs-copyfile.js:32:1)
    at Module._compile (module.js:600:30)
    at Object.Module._extensions..js (module.js:611:10)
    at Module.load (module.js:521:32)
    at tryModuleLoad (module.js:484:12)
    at Function.Module._load (module.js:476:3)
    at Function.Module.runMain (module.js:641:10)
    at startup (bootstrap_node.js:187:16)
    at bootstrap_node.js:605:3
Command: out/Release/node /home/mario/Git_projects/node/node/test/parallel/test-fs-copyfile.js
[05:59|% 100|+ 1971|-   1]: Done
Makefile:201: recipe for target 'test' failed
make: *** [test] Error 1

Do you think that It could be a good idea to work directly on the stable node 8.6.0 instead of 9.0.0 PRE to avoid this kind of problems?

lpinca · 2017-10-07T18:32:20Z

@Giovarco from your fork:

Switch to the master branch and pull the upstream changes.
Switch to your branch and rebase against master.
Rebuild and run the tests again.

jasnell · 2017-10-10T00:26:17Z

lib/events.js

+
+      const functionArray = obj[event];
+
+      for(let i = 0; i < functionArray.length; i++) {


This should allow for the simple case also:

ee.onMultiple({ event1() { }, event2() { } });

Not sure what that object represents. Is it like an array?

jasnell · 2017-10-10T00:27:37Z

test/parallel/test-event-emitter-onMultiple.js

+    await myEE.emit("event2");
+    await myEE.emit("event3");
+
+    assert.deepStrictEqual(ok, 3, "Some handlers were not executed");


Wrap the functions in the input with a common.mustCall() and this check becomes unnecessary.

e.g.

const input = { event1: [ common.mustCall() ], event2: [ common.mustCall(), common.mustCall() ] };

jasnell · 2017-10-10T00:28:12Z

Just noting... this would also need documentation.

Giovarco · 2017-10-12T19:56:31Z

@jasnell No problem. As soon as we're done with the code.

I had to delete my repo, so now this PR is orphan. I'll open another one PR, so we all can discuss there: #16169

Giovarco added 4 commits October 3, 2017 17:23

00f869c

- Add onMultiple function

ce6d340

- Add unit tests

696bbb7

- Change name of the unit test

aa2c3c6

- Use symbol on the unit test

nodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label Oct 5, 2017

Giovarco changed the title ~~Feature/multi listener~~ Adding multiple listeners at once Oct 5, 2017

benjamingr reviewed Oct 5, 2017

View reviewed changes

ff9becc

- Accept an object as input parameter now - Change unit tests accordingly

Giovarco mentioned this pull request Oct 7, 2017

From which commit should I start making a new feature? #16064

Closed

jasnell reviewed Oct 10, 2017

View reviewed changes

Giovarco mentioned this pull request Oct 12, 2017

events: add onMultiple function #16169

Closed

4 tasks

Giovarco closed this Oct 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding multiple listeners at once #15787

Adding multiple listeners at once #15787

Giovarco commented Oct 5, 2017 •

edited

Loading

mscdex commented Oct 5, 2017

Giovarco commented Oct 5, 2017

mscdex commented Oct 5, 2017 •

edited

Loading

benjamingr Oct 5, 2017

Giovarco Oct 6, 2017

bnoordhuis commented Oct 6, 2017

Giovarco commented Oct 6, 2017

bnoordhuis commented Oct 6, 2017

Giovarco commented Oct 6, 2017 •

edited

Loading

bnoordhuis commented Oct 6, 2017

Giovarco commented Oct 6, 2017 •

edited

Loading

Giovarco commented Oct 6, 2017 •

edited

Loading

mscdex commented Oct 6, 2017

Giovarco commented Oct 7, 2017

lpinca commented Oct 7, 2017

jasnell Oct 10, 2017

Giovarco Oct 12, 2017

jasnell Oct 10, 2017

Giovarco Oct 12, 2017

jasnell commented Oct 10, 2017

Giovarco commented Oct 12, 2017


		const functionArray = obj[event];

		for(let i = 0; i < functionArray.length; i++) {

Adding multiple listeners at once #15787

Adding multiple listeners at once #15787

Conversation

Giovarco commented Oct 5, 2017 • edited Loading

Checklist

Affected core subsystem(s)

mscdex commented Oct 5, 2017

Giovarco commented Oct 5, 2017

mscdex commented Oct 5, 2017 • edited Loading

benjamingr Oct 5, 2017

Choose a reason for hiding this comment

Giovarco Oct 6, 2017

Choose a reason for hiding this comment

bnoordhuis commented Oct 6, 2017

Giovarco commented Oct 6, 2017

bnoordhuis commented Oct 6, 2017

Giovarco commented Oct 6, 2017 • edited Loading

bnoordhuis commented Oct 6, 2017

Giovarco commented Oct 6, 2017 • edited Loading

Giovarco commented Oct 6, 2017 • edited Loading

mscdex commented Oct 6, 2017

Giovarco commented Oct 7, 2017

lpinca commented Oct 7, 2017

jasnell Oct 10, 2017

Choose a reason for hiding this comment

Giovarco Oct 12, 2017

Choose a reason for hiding this comment

jasnell Oct 10, 2017

Choose a reason for hiding this comment

Giovarco Oct 12, 2017

Choose a reason for hiding this comment

jasnell commented Oct 10, 2017

Giovarco commented Oct 12, 2017

Giovarco commented Oct 5, 2017 •

edited

Loading

mscdex commented Oct 5, 2017 •

edited

Loading

Giovarco commented Oct 6, 2017 •

edited

Loading

Giovarco commented Oct 6, 2017 •

edited

Loading

Giovarco commented Oct 6, 2017 •

edited

Loading