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.

Sign up for GitHub

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

Jump to bottom

AsyncIterator-ext Package #66

Open
jeswr opened this issue Apr 14, 2022 · 7 comments · May be fixed by #68
Open

AsyncIterator-ext Package #66

jeswr opened this issue Apr 14, 2022 · 7 comments · May be fixed by #68

Comments

@jeswr
Copy link
Collaborator

jeswr commented Apr 14, 2022 •
edited

In order to support some more niche use-cases I am coming across I was wondering if it is worth creating an AsyncIterator-ext package (see example below).

IMO the best way to go about this would be:

  • Create an AsyncIterator organisation
  • Move the AsyncIterator repo to a repo in this organisation [Requires @RubenVerborgh due to repo permissions]
  • Create a monorepo of extension components that get released under the @asynciterator/extension-name schema (I am happy to take responsibility for maintenance of this new repo)
  • Additionally export all of the extensions in the package @asynciterator/ext

The downside of having these in a separate package is that it is hard to use these functions as methods of an iterator (as if we just extended the existing AsyncIterator with an AsyncIteratorExt class then using a method like map would return an AsyncIterator class without any of the extension functionality).

An example of an export from that package would be the following maybeIterator function which returns undefined on emptyIterators and an iterator otherwise (the use case for me is to terminate a forward chaining procedure in reasoning components):

import { AsyncIterator } from 'asynciterator';

/**
 * @param source An AsyncIterator
 * @returns The AsyncIterator if it is not empty, otherwise undefined
 */
async function maybeIterator<T>(source: AsyncIterator<T>): Promise<null | AsyncIterator<T>> {
     // Avoid creating a new iterator where possible
  if ((source instanceof ArrayIterator || source instanceof BufferedIterator) && source._buffer.length > 0) {
     return source
  }
  if (source instanceof IntegerIterator) {
     return source.step >= 0 ? source.next > source.last : source.next : source.last
  }

  let item;
  do {
    if ((item = source.read()) !== null)
      return source.append([item]);
    await awaitReadable(source);
  } while (!source.done);
  return null;
}

function awaitReadable<T>(source: AsyncIterator<T>): Promise<void> {
  return new Promise<void>((res, rej) => {
    if (source.readable || source.done)
      res();

    function done() {
      cleanup();
      res();
    }

    function err() {
      cleanup();
      rej();
    }

    function cleanup() {
      source.removeListener('readable', done);
      source.removeListener('end', done);
      source.removeListener('error', err);
    }

    source.on('readable', done);
    source.on('end', done);
    source.on('error', err);
  });
}
@jeswr jeswr mentioned this issue Apr 14, 2022
Open
@RubenVerborgh
Copy link
Owner

I'm not opposed, but I wonder if that's needed.

AsyncIterator should be split into multiple files, and then people can just import what they need?

@jeswr
Copy link
Collaborator Author

jeswr commented Apr 14, 2022

As long as you are happy to include that kind of functionality in the main package I'm happy to split the files and add these extensions as separate files once all of the current work around perf. settles down.

@jacoscaz
Copy link
Collaborator

If we're talking about something for the next major release, I would also suggest getting rid of commodity prototypal methods entirely as they bind classes to one another, preventing tree-shaking.

@RubenVerborgh
Copy link
Owner

commodity prototypal methods entirely

Which ones?

@RubenVerborgh
Copy link
Owner

As long as you are happy to include that kind of functionality in the main package I'm happy to split the files and add these extensions as separate files once all of the current work around perf. settles down.

Indeed! Stay tuned; health and time permitting, I'll get to the perf issues and prepare the next release.

@jacoscaz
Copy link
Collaborator

Which ones?

@RubenVerborgh the fact that map(), filter(), union() transform(), prepend(), append() and so forth are all defined on the prototype of AsyncIterator means that the respective classes MappingIterator, UnionIterator, TransformIterator, etc... cannot be tree-shaken from one another even when only one of those subclasses is used. AsyncIterator is likely not big enough for this to become an actual issue but, given that we're adding specialized classes for performance reasons, perhaps it'd be a good thing to keep tree-shaking into account.

A potential solution would be to have top-level map(source, fn), filter(source, fn), transform(source, opts) functions, each returning instances their respective subclass. typed-immutable-map, which I've recently forked from an abandoned library and done some work on, uses such an approach.

API-wise this would be a major major breaking change, though. Not sure whether the gains in bundle size would be worth the refactoring costs.

health and time permitting

Hope you're ok! Health comes first!

@RubenVerborgh
Copy link
Owner

RubenVerborgh commented Apr 14, 2022 •
edited

Thanks! I think it's quite okay and in fact crucial to have these methods, for performance and other reasons. (If they're methods, then subclasses can redefine them for more optional ones.) Some iterators are just core and don't need shaking off (we're talking about 3–4 iterators).

@jeswr jeswr linked a pull request Apr 28, 2022 that will close this issue
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: maybeIterator RubenVerborgh/AsyncIterator
3 participants
@RubenVerborgh @jacoscaz @jeswr