Make iterators AsyncIterable, Closes #89

[rubensworks]

While this will likely come at the cost of lower performance compared to plain data-events, we should add this for use cases where code readability is preferred over performance.

[jeswr]

LGTM (barring the minor optimisation above)

Note that this logic achieves the same effect over in LDflex https://github.com/LDflex/LDflex-Comunica/blob/f01ece6fa8cee720061efd5ddd23ed82921555e4/src/ComunicaEngine.ts#L161-L215.

I will leave @RubenVerborgh to merge as I do not have rights to make a subsequent npm release.

[rubensworks]

@RubenVerborgh Could you have a look at this one? Will certainly make many comunica users very happy :-)

[RubenVerborgh]

@rubensworks So I first did f60cecc to minimize listener re-attachment within a single next iteration, but then I realize we could keep listeners attached in 43cedf1, and then realized we must do that to catch errors in between next calls. However, the test coverage is not complete for those cases yet.

[rubensworks]

I also had a pendingError-like approach in mind before, but I decided to not implement it like that before due to the chance of errors getting lost.
What could happen, is that the AsyncIterable is only being consumed partially, that an error is emitted after the last consumed element, and that the error will get lost because no further elements are getting consumed.
The alternative is that unhandled errors get thrown, but at least the errors do not get lost that way 😅

So I guess we'll have to choose between either errors to always be explicitly forwarded (but them sometimes getting lost on partial consumption), or errors never getting lost, but them sometimes being unhandled errors.

---

After writing this, I just realized that we could also implement our own unhandled exception approach. If we notice (somehow) that the AsyncIterable is not consumed anymore, but we have a pending error, that we throw an unhandled exception. This might give us the best of both worlds.

[RubenVerborgh]

After writing this, I just realized that we could also implement our own unhandled exception approach. If we notice (somehow) that the AsyncIterable is not consumed anymore, but we have a pending error, that we throw an unhandled exception. This might give us the best of both worlds.

I like the approach, except I'm not sure how to implement it with the ECMAScript AsyncIterator contract. How do I know that no next will be called, if the error arrives between next calls?
What I am missing is a destroy method on ECMAScript AsyncIterator.

[rubensworks]

How do I know that no next will be called, if the error arrives between next calls?

We may be able to do something by registering the created AsyncIterator into a FinalizationRegistry, but it feels finicky...

[RubenVerborgh]

Ooof yes. Or perhaps for now at a comment that we recommend adding an error listener to the iterator if you don't plan on fully consuming it (since you can't detach a for await created thing yourself, because you don't have a reference to it).

Or we could keep a list per iterator of the ESAsyncIterator instances created (or even allow only one), and then destroy on the iterator also destroys all of them.

Actually, destroy on the iterator might just work because it would also delete listeners.

[rubensworks]

@RubenVerborgh At last, I found some time to look into finalizing the unit tests and documenting the error events edge-case.

[RubenVerborgh]

That is quite amazing, thanks. So we good to go? Anything I should still do? (I see a review is pending, just checking if you need specific things.)

[rubensworks]

@RubenVerborgh Should be feature-complete AFAICS, so it should be ready to be merged if you agree :-)

[RubenVerborgh]

🤦 obvs I put that in on purpose to check your tests

[RubenVerborgh]

I was so pleased that my code turned out to be the one in the end, only the see the dreadful !== error.
You go ahead and merge it, you did the tests and that's the hard work!

[rubensworks]

Merged! :-)

@RubenVerborgh, before you release, it might make sense to include #105 as well.

[jacoscaz]

@rubensworks @RubenVerborgh brilliant!!!

[rubensworks]

@RubenVerborgh Could you publish a new release? I'd like to include it in the next major release of Comunica, which I intend to release soon.

[RubenVerborgh]

@rubensworks Published v3.9.0! Thanks for your patience and continued support.