AsyncEmitter #427
Open
AsyncEmitter #427
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lundibundi
reviewed
Jun 2, 2019
nechaido
reviewed
Jun 3, 2019
nechaido
reviewed
Jun 3, 2019
lundibundi
reviewed
Jun 3, 2019
tshemsedinov
commented
Jun 3, 2019
tshemsedinov
force-pushed
the
async-ee
branch
2 times, most recently
from
1d27b1e
to
4102ef3
Compare
belochub
reviewed
Jun 4, 2019
nechaido
reviewed
Jun 4, 2019
lundibundi
reviewed
Jun 5, 2019
lundibundi
reviewed
Jun 5, 2019
lundibundi
reviewed
Jun 5, 2019
lundibundi
reviewed
Jun 5, 2019
lundibundi
reviewed
Jun 5, 2019
lundibundi
reviewed
Jun 5, 2019
nechaido
reviewed
Jun 5, 2019
belochub
reviewed
Jun 6, 2019
|
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: |
|
Implemented in a different way. Now we hold AsyncEmitter {
events:
Map {
'eventName' => {
on: Set { listener<Function> },
once: Map { listener<Function> => wrapper<Function> }
},
}
} |
nechaido
reviewed
Jun 7, 2019
lundibundi
reviewed
Jun 9, 2019
lundibundi
reviewed
Jun 9, 2019
nechaido
reviewed
Jun 11, 2019
Co-Authored-By: Denys Otrishko <shishugi@gmail.com> Co-Authored-By: Mykola Bilochub <nbelochub@gmail.com> Co-Authored-By: Dmytro Nechai <nechaido@gmail.com>
lundibundi
approved these changes
Jun 15, 2019
lundibundi
reviewed
Jun 15, 2019
| const event = this.events.get(name); | ||
| if (!event) return 0; | ||
| const { on, once } = event; | ||
| return on.size + once.size; |
There was a problem hiding this comment.
Nit: can be simplified to:
return event ? event.on.size + event.once.size : 0;
nechaido
reviewed
Jul 3, 2019
| // Get or create event | ||
| // name <string> event name | ||
| // Returns: { on: <Set>, once: <Set> } } | ||
| event(name) { |
There was a problem hiding this comment.
I'd prefer to use Symbol('getListeners') for this, to make this property "private".
|
|
||
| class AsyncEmitter { | ||
| constructor() { | ||
| this.events = new Map(); |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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) => { |
There was a problem hiding this comment.
?
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(); | ||
|
|
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
Suggested change
| this.once(name, resolve); | |
| this.once(name, (...args) => resolve(args.length <= 1 ? args[0] : args) | |
| ); |
| }); | ||
|
|
||
| setTimeout(fn, 0); | ||
| await ae.once('e1'); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I suppose we need to hide counters when waiting for multiple event handlers to this abstraction.