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

AsyncEmitter #427

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

AsyncEmitter #427

wants to merge 3 commits into from

Conversation

tshemsedinov
Copy link
Member

ee.on('e1', async () => true);
await ee.emit('a1');

I suppose we need to hide counters when waiting for multiple event handlers to this abstraction.

lib/aee.js Outdated Show resolved Hide resolved
lib/aee.js Outdated Show resolved Hide resolved
lib/aee.js Outdated Show resolved Hide resolved
lib/aee.js Outdated Show resolved Hide resolved
lib/aee.js Outdated Show resolved Hide resolved
lib/aee.js Outdated Show resolved Hide resolved
@tshemsedinov tshemsedinov changed the title AsyncEventEmitter AsyncEmitter Jun 3, 2019
lib/async-emitter.js Outdated Show resolved Hide resolved
lib/async-emitter.js Show resolved Hide resolved
lib/async-emitter.js Outdated Show resolved Hide resolved
lib/async-emitter.js Outdated Show resolved Hide resolved
@tshemsedinov tshemsedinov self-assigned this Jun 3, 2019
@tshemsedinov tshemsedinov force-pushed the async-ee branch 2 times, most recently from 1d27b1e to 4102ef3 Compare June 4, 2019 07:58
metasync.js Outdated Show resolved Hide resolved
test/async-emitter.js Outdated Show resolved Hide resolved
test/async-emitter.js Outdated Show resolved Hide resolved
test/async-emitter.js Outdated Show resolved Hide resolved
test/async-emitter.js Outdated Show resolved Hide resolved
lib/async-emitter.js Outdated Show resolved Hide resolved
lib/async-emitter.js Show resolved Hide resolved
test/async-emitter.js Outdated Show resolved Hide resolved
test/async-emitter.js Outdated Show resolved Hide resolved
test/async-emitter.js Show resolved Hide resolved
test/async-emitter.js Show resolved Hide resolved
lib/async-emitter.js Outdated Show resolved Hide resolved
.eslintignore Outdated Show resolved Hide resolved
lib/async-emitter.js Outdated Show resolved Hide resolved
lib/async-emitter.js Outdated Show resolved Hide resolved
lib/async-emitter.js Outdated Show resolved Hide resolved
@tshemsedinov
Copy link
Member Author

I added tests that reveal problems and now it’s clear that we need to change data structure for wrappers. Maybe better store them mixed to functions: fn[symbolWrapper] = wrapper ? @nechaido @belochub @lundibundi

@tshemsedinov
Copy link
Member Author

Implemented in a different way. Now we hold on and once in different data structures for each event separately:

AsyncEmitter {
  events:
    Map {
      'eventName' => {
        on: Set { listener<Function> },
        once: Map { listener<Function> => wrapper<Function> }
      },
  }
}

@nechaido @belochub @lundibundi

lib/async-emitter.js Outdated Show resolved Hide resolved
lib/async-emitter.js Outdated Show resolved Hide resolved
lib/async-emitter.js Outdated Show resolved Hide resolved
lib/async-emitter.js Outdated Show resolved Hide resolved
lib/async-emitter.js Outdated Show resolved Hide resolved
lib/async-emitter.js Outdated Show resolved Hide resolved
test/async-emitter.js Show resolved Hide resolved
lib/async-emitter.js Outdated Show resolved Hide resolved
lib/async-emitter.js Outdated Show resolved Hide resolved
Co-Authored-By: Denys Otrishko <[email protected]>
Co-Authored-By: Mykola Bilochub <[email protected]>
Co-Authored-By: Dmytro Nechai <[email protected]>
const event = this.events.get(name);
if (!event) return 0;
const { on, once } = event;
return on.size + once.size;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can be simplified to:

return event ? event.on.size + event.once.size : 0;

// Get or create event
// name <string> event name
// Returns: { on: <Set>, once: <Set> } }
event(name) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to use Symbol('getListeners') for this, to make this property "private".


class AsyncEmitter {
constructor() {
this.events = new Map();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.events = new Map();
this.listeners = new Map();

if (!event) return Promise.resolve();
const { on, once } = event;
const promises = [...on, ...once].map(fn => fn(...args));
once.clear();
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to move this one line up, because some of the listeners may create additional once listeners.

metatests.test('AsyncEmitter on/emit', async test => {
const ae = new AsyncEmitter();

const fn = test.mustCall(async (a, b, c, d) => {
Copy link
Member

Choose a reason for hiding this comment

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

?

Suggested change
const fn = test.mustCall(async (a, b, c, d) => {
const fn = test.mustCall(async (...args) => { test.strictSame(args, [1, 2, 3, 4]) });


metatests.test('AsyncEmitter once', async test => {
const ae = new AsyncEmitter();

Copy link
Member

Choose a reason for hiding this comment

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

You should add a test, to ensure that emit args are forwarded to once listener.

once(name, fn) {
if (fn === undefined) {
return new Promise(resolve => {
this.once(name, resolve);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.once(name, resolve);
this.once(name, (...args) => resolve(args.length <= 1 ? args[0] : args)
);

});

setTimeout(fn, 0);
await ae.once('e1');
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants