feat: implement an async iterable request class

[chdh]

This PR implements an async iterable request class.

The new class IterableRequest is implemented as a super class of the normal Request class. It is an AsyncIterable providing an AsyncIterator and can be used with for await.

Usage:

const request = new IterableRequest("select 42, 'hello world'");
connection.execSql(request);
for await (const item of request) {
  console.log(item.row);
}

A unit test is included. The API documentation has yet to be written.

Implements #1550

[codecov]

Codecov Report

Attention: Patch coverage is 83.69565% with 15 lines in your changes missing coverage. Please review.

Project coverage is 78.39%. Comparing base (65ab37d) to head (9691e11).
Report is 16 commits behind head on master.

Files with missing lines	Patch %	Lines
src/iterable-request.ts	83.69%	9 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1644      +/-   ##
==========================================
- Coverage   79.01%   78.39%   -0.63%     
==========================================
  Files          93       91       -2     
  Lines        4876     4947      +71     
  Branches      937      948      +11     
==========================================
+ Hits         3853     3878      +25     
- Misses        720      768      +48     
+ Partials      303      301       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[arthurschreiber]

First of all, this is super exciting - thank you for working on this! ❤️

I haven't taken a super deep look into the implementation yet, but my idea for this was always to change the API for request execution to something like this:

// Create a new request, but without callback.
const request = new Request('select a, b, c from foobar');

// `using` so any resources are free'd to make the connection reusable in case some error happens in the user's code
using response = await connection.execSql(request);

// `.values` for a rows as arrays
// `.objects` for rows as objects
for await (const [a, b, c] of response.values()) {
  ...
}

Obviously, making such a profound change to the API is not simple, and there's going to be a lot of concerns around backwards compatibility and things like that.

[arthurschreiber]

Have you thought about implementing this through using events.on? I haven't tested this, but it would allow you to get rid of the custom iterator implementation.

  constructor(sqlTextOrProcedure: string | undefined, options?: IterableRequestOptions) {
    super(sqlTextOrProcedure, completionCallback, options);
    const controller = new AbortController();
    this.doneSignal = controller.signal;

    function completionCallback(error: Error | null | undefined) {
       controller.abort(error);
    }
  }

  [Symbol.asyncIterator](): AsyncIterator<IterableRequestItem> {
    return events.on(this, 'row', { signal: this.doneSignal });
  }

[chdh]

Thanks. I didn't know about events.on(). It does more or less the same as my controller and iterator classes.
I will refactor my code into using events.on().

[chdh]

Since Tedious still has to support Node 18, I had a look at that old implementation of event.on().
The close event was not yet implemented in that version. I can't find a proper way to end the iteration so that the remaining items in the FIFO are still processed. abortListener() calls errorHandler() which calls toError.reject(err) which rejects the waiting Promise,
Calling iterator.return() resolves the waiting Promise with done = true.

Should I use my implementation for old Node versions and event.on() for new Node versions?

[arthurschreiber]

config.options.useColumnNames is super bad API design, and I'd prefer if we could not propagate this nonsense. 😬

The reason why it's bad API design is that the config value changes the shape of the data that is being returned from tedious. Flipping this config basically from true to false or the other way around basically breaks all code that was written with the other value set.

Much better would be to have an API that allows the code to explicitly either use objects or use arrays.

[chdh]

I agree. But as you wrote before, this would be a profound change, beyond the scope of this PR.

[arthurschreiber]

I guess this is similar to highWaterMark on regular streams? I.e. once the buffer (here called fifo) reaches that size, no more data will be buffered and the underlying request will be paused?

[chdh]

Yes. Do you suggest to rename it to this.fifoHighWaterMark? And also rename the option parameter?