search

RubenVerborgh / AsyncIterator

An asynchronous iterator library for advanced object pipelines in JavaScript
https://rubenverborgh.github.io/AsyncIterator/docs/
Other
48 stars 7 forks source link

BREAKING: Call _end via read method #43

Closed jeswr closed 2 years ago

jeswr commented 2 years ago

resolves #25 #35

coveralls commented 2 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling ed8d4918ae676e2fa09f12e8e147885ce2ce6f68 on jeswr:breaking/end-on-read into 7a089e36e8c33c35b27b34cd670518f2fa51acce on RubenVerborgh:main.

jeswr commented 2 years ago

@rubensworks - in your implementation of toArray you removed the listeners rather than destroying the iterator when the limit was reached - is there a particular reason for this?

You will see that I have changed this behavior in https://github.com/RubenVerborgh/AsyncIterator/pull/43/commits/ed8d4918ae676e2fa09f12e8e147885ce2ce6f68 - this is because as discussed here removing a 'data' listener should not pause the stream, so in the case of your tests with infinite iterators the test suite was entering an infinite loop. An alternative is to call .pause inside toArray; but I would hazard against this as it would give unexpected behavior if other data listeners are attached.

RubenVerborgh commented 2 years ago

@jeswr Thanks for your work.

There are quite some unrelated changes in this PR, which makes it hard for me to assess. Could we do this one step by step, possibly with different PRs for things like linting etc.? Changes like this one are a bit distracting; I'd want to focus on one thing at a time.

rubensworks commented 2 years ago

@rubensworks - in your implementation of toArray you removed the listeners rather than destroying the iterator when the limit was reached - is there a particular reason for this?

The reasoning here was that the iterator could still be used after calling toArray(). For example for splicing of multiple arrays from the iterator, or perhaps even resuming the stream after first consuming an array. But given the discussion in https://github.com/comunica/comunica/issues/950, it might make sense to change this though.

RubenVerborgh commented 2 years ago

In general, I like to leave options open. So the idea is that, after toArray is finished. the derived iterator has stopped existing and the source iterator is free again. This default enables cases such as the ones @rubensworks describes.

But destroySource: true might be appropriate.