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

breaking: end on read (part 1) #45

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jeswr
Copy link
Collaborator

@jeswr jeswr commented Mar 14, 2022

resolves #25
resolves #35

Part of #43 without any lint or limit changes

@coveralls
Copy link

Coverage Status

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

@@ -1405,15 +1384,29 @@ export class MultiTransformIterator<S, D = S> extends TransformIterator<S, D> {
// Remove transformers that have ended
const transformerQueue = this._transformerQueue, optional = this._optional;
let head, item;
while ((head = transformerQueue[0]) && head.transformer.done) {
while ((head = transformerQueue[0]) !== undefined) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I feel the changes here could do with a little explanation:

In short - since end is now triggered by calling .read, we need to make sure that we now always have triggered at least one .read call before checking if the iterator is .done.

At the same time I added the ability to add data from multiple consecutive Transformers in the queue so long as they have data ready; this can reduce excessive calls to _read if the transformers in the queue only produce a small number of elements each.

@jeswr
Copy link
Collaborator Author

jeswr commented Mar 15, 2022

@RubenVerborgh this is ready for review - apparently I don't have permissions to set you as a reviewer over here ...

@RubenVerborgh
Copy link
Owner

Copying some test code by @jeswr from #75

        // TODO: Re-enable this section check for once https://github.com/RubenVerborgh/AsyncIterator/pull/45
        // is merged in order to check that destruction occurs immediately rather than over several ticks
        // it('should destroy when closed before being read after map', async () => {
        //   iterator.map(x => x).close();
        //   // TODO: Re-enable this same-tick check once https://github.com/RubenVerborgh/AsyncIterator/pull/45
        //   // is merged
        //   // iterator.destroyed.should.be.true;

        //   // Wait a tick
        //   await Promise.resolve();
        //   iterator.destroyed.should.be.true;
        //   console.log('b', iterator._state, iterator.open, iterator.closed, iterator.ended, iterator.destroyed)
        // });

        // it('should destroy when closed before being read after map then filter', async () => {
        //   const iter = iterator.map(x => x);
        //   iter.filter(x => true).close();

        //   // TODO: Re-enable this same-tick check once https://github.com/RubenVerborgh/AsyncIterator/pull/45
        //   // is merged
        //   // iterator.destroyed.should.be.true;
        //   // it.destroyed.should.be.true;

        //   // Wait a tick
        //   await Promise.resolve();
        //   iterator.destroyed.should.be.true;
        //   iter.destroyed.should.be.true;
        // });

@jeswr
Copy link
Collaborator Author

jeswr commented Aug 14, 2022

Note to self: whenever I revisit this PR, I should add some more tests, and review the code, to make sure that error events are never emitted before the iterator is started.

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