Implement listener subscription as async iterator protocol #20

lorenzofox3 · 2018-01-02T16:38:24Z

Note:

I fail to run xo (it seems the parser used does not understand await-for-of statement)
tests are implemented but I am not able to run them. I have been passing some hours trying to configure ava with babel without success. Please help me! However the code "manually" works in implementing engines (see jsfiddle with chrome 63 or firefox 57)

…lose #3

sindresorhus · 2018-01-02T20:31:43Z

I fail to run xo (it seems the parser used does not understand await-for-of statement)

Just npm i -D babel-eslint and add the following to the bottom of package.json:

"xo": {
	"parser": "babel-eslint"
}

tests are implemented but I am not able to run them.

Try adding the following below babel.plugins in package.json:

"presets": [
	"@ava/stage-4"
]

lorenzofox3 · 2018-01-03T00:40:08Z

Great ! thanks. Still unable to run with a for await statement though. However it seems to be yet another issue with the transpiled code:

no issue when manually iterating over the iterator (through next method)
error says [Symbol.iterator] is undefined whereas I am pretty sure there is no issue on supporting platforms (see jsFiddle) so I believe something is wrong with the transpiled code

//in async function context
const iterator = emitter.on('foo');
const {value,done} = await iterator.next(); // This works with transpiled and not transpiled code

//this does not work with transpiled code but works on supporting platforms
for await (let result of emitter.on('foo')){
  //
}

novemberborn

@lorenzofox3 very neat! I wonder though whether this should be landed before there's a Node.js version that supports asynchronous iterators. Can the iteration protocol be implemented without using async function *?

novemberborn · 2018-01-03T13:30:35Z

index.js

+			if (queue.length > 0) {
+				yield queue.shift();
+			} else {
+				yield await emitter.once(eventName);


This shouldn't yield, queue will contain the data of the next event after await returns.

Alternatively you could await a new promise, and expose its resolve() method to the listener created above. That way you don't need to add an additional listener.

you are right the yield is superfluous.
However I don't think exposing the resolve of the new promise on each iteration is a good idea: As the listener above is created once I would need to create a binding in its closure and reassign this binding to the resolve of the new promise on each iteration. That seems a bit awkward and unsafe, or I am missing something.

novemberborn · 2018-01-03T13:31:51Z

index.js

-		this._getListeners(eventName).add(listener);
-		return this.off.bind(this, eventName, listener);
+
+		if (typeof listener === 'function') {


Should this check the number of arguments passed, rather than the listener type? It seems a little too easy to accidentally create an asynchronous iterator otherwise.

As you wish. Although it is a public method with a specific contract so I think this alternative is better. Otherwise it means I'll need to check for "arguments" which is not correct in strict mode; Or use the spread operator on the signature which makes the contract less obvious in my opinion

dinoboff · 2018-01-03T14:16:04Z

You can use node 9 with --harmony flag or node 10 nightly.

For transpiler, I think TS has better async-iterator support than babel; it had the last time I checked.

dinoboff · 2018-01-03T14:24:53Z

Until better support for async-iterator, it would be better to set it up as a helper in an other module, e.g. emittery/iterator.js, or an other package. It would either monkey patch / extend Emittery or be function taking an event emitter and an event name.

lorenzofox3 · 2018-01-03T15:50:07Z

I have updated to pull request so the async iterator syntax behaviour has its own file.
Note I am still having trouble running the test here:

all tests run for legacy.js and main.js
I have added a npm run test:next (for nodejs 9 >) to run the tests without any transpilation (on modern engines). However ava stills transpiles and fails as the default preset does not understand for await statement. I have tried to add an ava configuration to bypass transpilation as suggested in the documentation. However this configuration becomes invalid for the regular case (npm test). Any Idea ?

This reverts commit d9b617a.

novemberborn · 2018-01-04T15:23:11Z

@lorenzofox3 I've been playing with this a bit. I hope you don't mind if I push my commits onto your branch.

Async iterators can be implemented without async function *, see https://jakearchibald.com/2017/async-iterators-and-generators/#a-shorter-implementation. This means we don't need to transpile the source. We can return an async iterator that is usable even on Node.js versions that don't have for-await-of.

babel-plugin-transform-async-generator-functions still requires Symbol.asyncIterator to exist. Perhaps this caused your test failure?

I think we still need to resolve:

How do we guard against emitter.on('event', myUndefinedListener) accidentally returning an iterator and its associated memory leaks? Perhaps the iterator doesn't actually subscribe until next() is called?
Should onAny() return an iterator?
Should breaking out of the for-await-of unsubscribe the iterator?
Need to update the TypeScript definition and the README

lorenzofox3 · 2018-01-04T16:59:45Z

@lorenzofox3 I've been playing with this a bit. I hope you don't mind if I push my commits onto your branch.

Don't worry it is all good !

Async iterators can be implemented without async function *, see https://jakearchibald.com/2017/async-iterators-and-generators/#a-shorter-implementation. This means we don't need to transpile the source. We can return an async iterator that is usable even on Node.js versions that don't have for-await-of.

Agree but the whole point if I understood correctly was to get the syntactic sugar of the for-await-of .
So

People who want to use it now will likely have support on their platform and does not need to transpile anyway. (That's why I was quite enthusiastic with the different files/packages approach)
Support will soon be quite wide.
I feel generators are more appropriate for such use cases (implementation is easier to read than going with the low level interface of the iterator protocol where you need to be careful to release resources etc.) - see Reginald Braithwaite article. Of course that is only a matter of preferences and I will let you make the choice.

babel-plugin-transform-async-generator-functions still requires Symbol.asyncIterator to exist. Perhaps this caused your test failure?

It is more my inability to run AvA with different configurations, I wanted to:

run the main tests with the default settings (default Emittery implmentation)
run natively, with no transpilation, on node 9 --harmony the tests for the async iterators implementation. This requires to set a different configuration so AvA bypass the transpilation. However this configuration becomes invalid for the first use case. Bottom's line: I am not confortable with AvA :)

Regarding your implementation:
I am not a big fan of relying on a listener closure (your flush function) being reassigned on every iteration. See my previous comment:

you are right the yield is superfluous.
However I don't think exposing the resolve of the new promise on each iteration is a good idea: As the listener above is created once I would need to create a binding in its closure and reassign this binding to the resolve of the new promise on each iteration. That seems a bit awkward and unsafe, or I am missing something.

Again that is a matter of preference I guess.

How do we guard against emitter.on('event', myUndefinedListener) accidentally returning an iterator and its associated memory leaks? Perhaps the iterator doesn't actually subscribe until next() is called?

Right I had not thought of an undefined variable. Then, I think another method like for await (const d of emitter.from('event)){} would be better. But your solution seems good too.

Should onAny() return an iterator?

probably.

Should breaking out of the for-await-of unsubscribe the iterator?

yes (if it follows the same semantic than a the for of loop). It will call the the return method of the iterator or the finally clause of the generator (see the link above from Reginald).

dinoboff · 2018-01-04T22:26:30Z

index.js

+		},
+		return() {
+			off();
+			queue = null;


It should be async too, take a value argument and return {done: true, value}.

https://tc39.github.io/proposal-async-iteration/#sec-asynciterator-interface

novemberborn · 2018-01-05T13:39:27Z

Agree but the whole point if I understood correctly was to get the syntactic sugar of the for-await-of.

@lorenzofox3 that still works even with the manual implementation. The benefit though is that the code is directly compatible with Node.js 8.

It is more my inability to run AvA with different configurations, I wanted to:

run natively, with no transpilation, on node 9 --harmony the tests for the async iterators implementation. This requires to set a different configuration so AvA bypass the transpilation. However this configuration becomes invalid for the first use case. Bottom's line: I am not confortable with AvA :)

This would be possible by wrapping the babel-plugin-transform-async-generator-functions plugin and not applying it under Node.js 9. It'd be a nice way of ensuring the implementation is compatible with V8 rather than Babel.

I am not a big fan of relying on a listener closure (your flush function) being reassigned on every iteration

Awaiting .once() is more elegant, yes, though we'd have to make sure we don't enqueue the same value in the existing .on() listener. I don't see an elegant way of doing that, which lead me down this path.

I think another method like for await (const d of emitter.from('event)){} would be better.

I do like the idea of using different method names! Especially since these iterators aren't immediately useful on Node.js <= 8. I'm struggling with the name though. I can definitely see Emittery being subclassed a lot, and from seems awfully generic to force upon subclasses.

Sticking with the idea of making the iterator lazy, we could add a .buffer() function which starts buffering events before iteration begins. Perhaps that gives us the best of both worlds.

sindresorhus · 2018-01-05T14:12:38Z

I can definitely see Emittery being subclassed a lot, and from seems awfully generic to force upon subclasses.

.fromEvent('foo') ?

novemberborn · 2018-01-05T16:42:46Z

.fromEvent('foo') ?

It's not necessarily clear that returns an iterator, though the same applies to .on('foo').

sindresorhus · 2018-01-05T16:50:39Z

.asyncIteratorForEvent(‘foo’) ?

dinoboff · 2018-01-06T01:01:16Z

.events('foo')?

novemberborn · 2018-01-06T13:48:10Z

I like .events(eventName)! We could then also add .anyEvent().

…the same event This clears 'queue' before the iterator pushes to it, which shouldn't cause a crash.

Feature-detect for-await-of support, and only transpile this feature when necessary. With Node.js 9 you can use the following to run tests with the native implementation: ```console $ node --harmony_async_iteration ./node_modules/.bin/ava ```

novemberborn · 2018-01-06T18:05:53Z

I've done some more work on this now. I'd like to land #25 so clearListeners() can also signal the iterators that they'll be done once fully consumed.

This syntax is transformed using async-to-generator, not async-generator-functions.

Follows https://tc39.github.io/proposal-async-iteration/#sec-asynciterator-interface

novemberborn · 2018-01-07T15:32:13Z

I'd like to land #25 so clearListeners() can also signal the iterators that they'll be done once fully consumed.

Conceptually this may conflict with regular listener behavior. #26 makes it explicit that emit() calls listeners added before emit() was invoked. clearListeners() wouldn't impact this for regular listeners, however we'd end up with race conditions where the final event may or may not be observed in the asynchronous iterator.

emitSerial() may cause similar race conditions, where the event data is not delivered to the iterator until after a later call to emit(). Perhaps data should be delivered to the iterators synchronously. This is fine since it's only read asynchronously anyway.

novemberborn · 2018-01-08T16:07:08Z

I'd like to land #25 so clearListeners() can also signal the iterators that they'll be done once fully consumed.

Conceptually this may conflict with regular listener behavior. #26 makes it explicit that emit() calls listeners added before emit() was invoked. clearListeners() wouldn't impact this for regular listeners, however we'd end up with race conditions where the final event may or may not be observed in the asynchronous iterator.

emitSerial() may cause similar race conditions, where the event data is not delivered to the iterator until after a later call to emit(). Perhaps data should be delivered to the iterators synchronously. This is fine since it's only read asynchronously anyway.

This is no longer a concern with the changes in #27. clearListeners() will prevent the listener from being called, no matter if invocations are pending. So when all that lands I'll fix the merge conflicts and implement the signaling from clearListeners() so the asynchronous iterators know they shouldn't expect newer events.

The TS definition for anyEvents() needs to be updated given #28.

novemberborn · 2018-01-09T14:46:02Z

emitSerial() may cause similar race conditions, where the event data is not delivered to the iterator until after a later call to emit(). Perhaps data should be delivered to the iterators synchronously. This is fine since it's only read asynchronously anyway.

Actually this does bother me. Will explore synchronously delivering data to the iterators, before other listeners are invoked.

clearListeners() now signals iterators to not expect further items, without clearing the current queue. Clarify that clearListeners() also impacts iterators. Similarly include the number of active iterators in listenerCount(). Enqueue new iterator items synchronously, before listeners are called.

novemberborn · 2018-01-20T17:39:42Z

I've resolved the merge conflicts. Iterators are now fed events without using the listener mechanism, given my earlier musings.

novemberborn · 2018-01-21T15:24:23Z

@dinoboff does the AsyncIterableIterator usage in the TS definition mean consumers will have to load the esnext.asynciterable library in their projects? Is this an OK thing to require? Or should we output a definition file with AsyncIterableIterator included?

dinoboff · 2018-01-23T15:28:33Z

@novemberborn I doubt it would be required. I will test it.

sindresorhus · 2018-02-17T19:01:16Z

@dinoboff Did you have a chance to test it? :)

novemberborn · 2018-02-18T14:09:50Z

I suspect that even if this is an issue with TS 2.7 it'll soon be moot, since async iterators are now stage 4.

lorenzofox3 · 2018-02-18T15:05:06Z

buy the way guys, I have published array like operators on asynchronous iterable ;)

dinoboff · 2018-02-19T09:22:02Z

@novemberborn tries with:

mkdir tmp
cd tmp
npm init -y
npm i typescript
npm i lorenzofox3/emittery

I used:

// index.ts
import Emittery = require('emittery');

const ee = new Emittery();

ee.emit('foo', 'bar');

When compiling with npx tsc index.ts, I get:

node_modules/emittery/Emittery.d.ts(33,30): error TS2304: Cannot find name 'AsyncIterableIterator'.
node_modules/emittery/Emittery.d.ts(81,15): error TS2304: Cannot find name 'AsyncIterableIterator'.
node_modules/emittery/Emittery.d.ts(139,61): error TS2304: Cannot find name 'AsyncIterableIterator'.
node_modules/emittery/Emittery.d.ts(140,54): error TS2304: Cannot find name 'AsyncIterableIterator'.
node_modules/emittery/Emittery.d.ts(147,15): error TS2304: Cannot find name 'AsyncIterableIterator'.

ps: It works with npx tsc index.ts -lib es6,esnext.asynciterable but not with npx tsc index.ts -lib es2018 (might have to wait for ts 2.8 to get the correct lib for ES 2018).

novemberborn · 2018-02-19T17:07:47Z

might have to wait for ts 2.8 to get the correct lib for ES 2018

I suspect so.

Question then is whether we wait or just document this.

sindresorhus · 2018-02-19T18:43:18Z

I think we should document the behavior and land this PR. When TS 2.8 is out we can just update the docs.

sindresorhus · 2019-06-11T13:32:59Z

@lorenzofox3 Would you be interested in finishing this?

lorenzofox3 · 2019-06-11T18:22:02Z

sure. give me until Monday

lorenzofox3 · 2019-06-12T14:55:17Z

Actually what do you expect exactly ? If I understand correctly @novemberborn had not merged his continued work due to a typescript issue. So what am I supposed to do ?

I think at this point and regarding the size of library, the age of the PR and the fact it has no dependency, it might be easier to (re)write with modern ES (and maybe directly in typescript as you want type declarations) and let the transpilation to the consumer of the library.

Who is actually using legacy.js ? If one wants to build a legacy application using emittery it will anyway probably transpile it himself to avoid the duplication of the async to generator babel helper shipped within legacy.js ?

I find the set up quite sophisticated and not very dev friendly for a somewhat simple library

sindresorhus · 2019-06-14T16:39:46Z

If I understand correctly @novemberborn had not merged his continued work due to a typescript issue.

The TypeScript thing should not be an issue anymore.

Who is actually using legacy.js ?

legacy.js is gone in master.

I think the remaining work is to fix the merge conflicts and document that the methods that return an async iterator require Node.js 12.

lorenzofox3 · 2019-06-14T18:13:36Z

I think the remaining work is to fix the merge conflicts and document that the methods that return an async iterator require Node.js 12.

Actually with the current implementation (ie latest commit on this PR), what requires a given version of node (8.10 with flags or above) is the for await statement (used by the consumer only). Note however that this implementation use arguments which if I am not wrong is not allowed in strict mode (and therefore in type=module script).

However if you want to swap with a more modern implementation (using async generator) we will need to drop the support for node 8 as the code of emittery itself would throw syntax error.

sindresorhus · 2019-06-15T06:46:46Z

Note however that this implementation use arguments which if I am not wrong is not allowed in strict mode (and therefore in type=module script).

You can use ...args instead, or better yet, just explicitly check the arguments.

However if you want to swap with a more modern implementation (using async generator) we will need to drop the support for node 8 as the code of emittery itself would throw syntax error.

I don't want that yet. We can open an issue about it after this PR is merged so we can move to a async generator in the future.

lorenzofox3 · 2019-06-18T16:18:32Z

closed in favor of #40 (I had deleted the fork repo)

lorenzofox3 added 2 commits January 2, 2018 17:24

feat: register event listener with asynchronous iterator protocol - c…

5656357

…lose #3

keep single line block formatting

78cbcd6

fix xo and test setting

b50ec91

lorenzofox3 added 2 commits January 3, 2018 01:43

enable xo

c904739

removed useless eslint flag comments

c951511

novemberborn reviewed Jan 3, 2018

View reviewed changes

split async iterator behavior to different file

d9b617a

novemberborn added 2 commits January 4, 2018 14:42

Revert "split async iterator behavior to different file"

cac49ab

This reverts commit d9b617a.

Merge branch 'master' into pr/20

ade1383

Implement async iterator protocol without generators, fix tests

a4d414d

dinoboff reviewed Jan 4, 2018

View reviewed changes

novemberborn added 6 commits January 6, 2018 14:57

Fix linting error

a6a3476

Simplify next() implementation

e91559c

Handle edge case where return() is called by an earlier listener for …

c4ca867

…the same event This clears 'queue' before the iterator pushes to it, which shouldn't cause a crash.

Implement return() according to spec

e18c260

Remove unnecessary tsconfig files in test fixtures

edb7ac5

Change how TypeScript is loaded in types test

756b47a

novemberborn added 2 commits January 6, 2018 17:03

Tweak AVA's Babel options

3f9aa37

novemberborn added 2 commits January 7, 2018 14:40

Fix for-await transpilation

361cddd

This syntax is transformed using async-to-generator, not async-generator-functions.

Ensure async iterators return non-promise values

735ee10

Follows https://tc39.github.io/proposal-async-iteration/#sec-asynciterator-interface

sindresorhus changed the title ~~implement listener subscription as asynchronous iterator protocol~~ Implement listener subscription as async iterator protocol Jan 8, 2018

novemberborn added 4 commits January 20, 2018 16:39

Merge branch 'master' into pr/20

6d73663

Tweak iterator implementation now that scheduling is more consistent

0afc521

Remove wrongly placed documentation

4cdcd6f

lorenzofox3 closed this Jun 18, 2019

Implement listener subscription as async iterator protocol #20

Implement listener subscription as async iterator protocol #20

Conversation

lorenzofox3 commented Jan 2, 2018

sindresorhus commented Jan 2, 2018

lorenzofox3 commented Jan 3, 2018

novemberborn left a comment

Choose a reason for hiding this comment

novemberborn Jan 3, 2018

Choose a reason for hiding this comment

lorenzofox3 Jan 3, 2018

Choose a reason for hiding this comment

novemberborn Jan 3, 2018

Choose a reason for hiding this comment

lorenzofox3 Jan 3, 2018

Choose a reason for hiding this comment

dinoboff commented Jan 3, 2018 • edited

dinoboff commented Jan 3, 2018

lorenzofox3 commented Jan 3, 2018 • edited

novemberborn commented Jan 4, 2018 • edited

lorenzofox3 commented Jan 4, 2018

dinoboff Jan 4, 2018 • edited

Choose a reason for hiding this comment

novemberborn commented Jan 5, 2018

sindresorhus commented Jan 5, 2018

novemberborn commented Jan 5, 2018

sindresorhus commented Jan 5, 2018

dinoboff commented Jan 6, 2018

novemberborn commented Jan 6, 2018 • edited

novemberborn commented Jan 6, 2018

novemberborn commented Jan 7, 2018

novemberborn commented Jan 8, 2018

novemberborn commented Jan 9, 2018

novemberborn commented Jan 20, 2018

novemberborn commented Jan 21, 2018

dinoboff commented Jan 23, 2018

sindresorhus commented Feb 17, 2018

novemberborn commented Feb 18, 2018

lorenzofox3 commented Feb 18, 2018

dinoboff commented Feb 19, 2018 • edited

novemberborn commented Feb 19, 2018

sindresorhus commented Feb 19, 2018 • edited

sindresorhus commented Jun 11, 2019

lorenzofox3 commented Jun 11, 2019

lorenzofox3 commented Jun 12, 2019

sindresorhus commented Jun 14, 2019

lorenzofox3 commented Jun 14, 2019

sindresorhus commented Jun 15, 2019

lorenzofox3 commented Jun 18, 2019

dinoboff commented Jan 3, 2018 •

edited

lorenzofox3 commented Jan 3, 2018 •

edited

novemberborn commented Jan 4, 2018 •

edited

dinoboff Jan 4, 2018 •

edited

novemberborn commented Jan 6, 2018 •

edited

dinoboff commented Feb 19, 2018 •

edited

sindresorhus commented Feb 19, 2018 •

edited