apache / incubator-annotator

Apache Annotator provides annotation enabling code for browsers, servers, and humans.
https://annotator.apache.org/
Apache License 2.0
219 stars 44 forks source link

sync vs async API #81

Closed Treora closed 3 years ago

Treora commented 4 years ago

As discussed in the call yesterday, I open this issue to park thoughts about whether/how to provide synchronous APIs, mainly future consideration.

So far, we have been using async iterables/generators as the system for returning a selector’s matches to the caller. The intention of this approach is to not block the thread for too long when dealing with e.g. a fuzzy text search (once that’s implemented) in a large document:

In our planned modular system, with support for different selector types and different implementations for anchoring them, some implementations may want to use the asynchronous approach, while others are quick enough to run synchronously. So far, our code only uses synchronous implementations, but it exposes them as asynchronous functions so that in the future one could swap out an implementation for an async one, and we can pass functions around without needing to distinguish between sync and async ones. Unfortunately, as Javascript lacks a way to turn a resolved promise into its value synchronously, a sync implementation with an async API cannot be wrapped to make it sync again.

Now a question may be whether, for situations where the implementations are synchronous anyway, it is a burden for users to have to use the asynchronous API when it is not needed. If so, one option would be to provide a sync and and async API, much like NodeJS does for many of its functions. It may require some reorganisation/duplication in our code and documentation, but we could consider this if the async approach is a show-stopper for some (potential) users; do leave a reply if that is the case for you.

tilgovi commented 4 years ago

I propose we start by making some attempt to have core pieces of our reusable code allow for synchronous iterators, but keep the asynchronous consumer API.

I started experimenting with something like this, that we could use in our definitions for cartesian product or our matcher functions.

type AbstractIterable<T> = AsyncIterable<T> | Iterable<T>;

export function isAsyncIterable<T>(
  iterable: AbstractIterable<T>,
): iterable is AsyncIterable<T> {
  return typeof iterable[Symbol.asyncIterator] === 'function';
}

export function makeAsync<T>(iterable: AbstractIterable<T>): AsyncIterable<T> {
  if (isAsyncIterable<T>(iterable)) {
    return iterable;
  }

  return {
    [Symbol.asyncIterator]() {
      const iter = iterable[Symbol.iterator]();
      return {
        next() {
          return Promise.resolve(iter.next());
        },
      };
    },
  };
}
Treora commented 4 years ago

Looks good. I’d be fine with adopting this approach, but equally fine with just keeping it in mind for when we have a need/requests for synchronous use.

tilgovi commented 3 years ago

Closing this. Sounds like we've got ideas for supporting synchronous APIs, but we don't have any urgency around it.