Update readable-stream to get asyncIterator

[jleedev]

I would like to write this:

for await (const row of mysql.query(…).stream()) { … }

Installing readable-stream@latest adds this feature.

However, version 3 of readable-stream only supports Node 6 and later, while version 2 supports all Node.js version from 0.8. You indicate that you support Node.js 0.6 or higher. The oldest Node.js version supported by upstream is 6 (soon to be 8 in less than a month), so it's probably reasonable to drop support for very old versions.

[dougwilson]

Hi @jleedev ! Being that this is a base driver, the goal is to support all possible Node.js versions, even when the Node.js mantainers no longer support them. This is because there are vendors that will provide support for older versions of Node.js, including backporting security fixes, etc.

If you want to use the newest readable-stream with query results, a simple solution can be to just pipe the stream from this module into the newer version of a pass through stream:

for await (const row of mysql.query(…).stream().pipe(new stream.PassThrough({objectMode: true}))) { … }

Ideally the original .stream() method should have never landed by using a module like readable-stream, as it should have instead used the in-built streams directly such that the interface would actually match the version of Node.js it is running on, automatically bringing in the new features that Node.js adds to stream (for example, we use the in-built EventEmitter, not an old copy).

This would be a breaking change, but is the change that should be made, not simply upgrading the readable-stream module and then just having this same issue in another future Node.js version, if that makes sense.

That said, hopefully the code snippet above is workable for you. Otherwise, at some point I can work on removing the readable-stream module altogether (or you're welcome to volunteer to do this as well).

[jleedev]

Yes, thanks for the snippet. I tried new Readable().wrap and it was failing mysteriously.

I'm not sure I understand why removing readable-stream would be a breaking change, but it nonetheless passes all the tests on Node 11. It would also be a matter of making sure it doesn't break on any version of Node that you support?

[jleedev]

In fact, it apparently works (tests pass) all the way back to Node 0.10, which introduced the stream.Readable class. Before that I imagine we would still need the readable-stream library.

[dougwilson]

Hi @jleedev so the reason it is likely a breaking change is because the API returned from .stream() is currently locked to whatever readable-stream is providing, and a change to use the native stream would alter the behavior for existing users. The tests around .stream() are very basic and exercise only the very top level features.

Just as an example: if one were to swap readable-stream out for the native stream, on Node.js 6 the order in which the 'data' and 'readable' events fire on the stream are reversed, which can be really hard to debug if this breaks something. Node.js put this in a major release for this reason.

Another issue is that in readable-stream v2, all errors it generated had a message only and no code property. This meant that you have to key off the message to determine the type of stream error. The error messages from streams vary from Node.js version to version, so a fallback to the native implementation will change the errors that are being emitted, causing another point of breaking.

Ultimately that above is speculation without working on a change to this module and verifying these things to know for sure, but hopefully it is some insight as to why the test suite may still be passing even with a breaking change lurking. Test suites unfortunately only catch regressions that they were programmed to check for, and the streams tests are very lacking currently.

As for the code snippet, was the one I showed not working for you? If it wasn't, are you getting an error at all, or can you describe what happened when you run it?

[jleedev]

It only returned the first row:

> for await (const row of conn.query("select * from t").stream().pipe(new stream.PassThrough({objectMode: true}))) { console.log(row) }
RowDataPacket { id: 1, data: '0' }
RowDataPacket { id: 2, data: '1' }
RowDataPacket { id: 3, data: '2' }
RowDataPacket { id: 4, data: '3' }
RowDataPacket { id: 5, data: '4' }
RowDataPacket { id: 6, data: '5' }
RowDataPacket { id: 7, data: '6' }
RowDataPacket { id: 8, data: '7' }
RowDataPacket { id: 9, data: '8' }
RowDataPacket { id: 10, data: '9' }
undefined
> for await (const row of new stream.Readable({objectMode:true}).wrap(conn.query("select * from t").stream())) { console.log(row) }
RowDataPacket { id: 1, data: '0' }
undefined
> 

[dougwilson]

Your output shows it returned 10 rows for the snippet I gave you.

[jleedev]

Yes, your snippet is working.

I had first tried new stream.Readable({objectMode:true}).wrap(…) to wrap the stream with the standard library version, and it emitted the first row then stopped.

[dougwilson]

Weird, I never used .wrap before, and I was just looking at the docs and it makes it sound like it only works in streams that are Node.js 0.8 and below style. Since the returned stream here is from readable-stream module I guess it is not an "old style" stream as the docs say, though they don't really say what exactly that means.

[jleedev]

Fair enough, thanks for trying to figure it out.

I appreciate that swapping out the entire stream library would be a risky change, and a workaround exists for my use case.

[dougwilson]

No problem. So this is what I am thinking right now

(1) you got a work around (yay)
(2) i do want to just get readable-stream removed as part of the 2.0 work
(3) I'm going to investigate why .wrap is not working to see if that is expected or there is something this module can fix to make it work

[dougwilson]

I wanted to update here that I am planning to release a 2.x minor next week and then get to work on a 3.0 so this should get resolved with that one way or another (even if it is just getting readable-stream dep updated to latest).

[sha1n]

I managed to create a working async-iterator using Readable.wrap (node 14.16.1, mysql 2.18.1). However, the first result I get is an OkPacket object that I have to manually ignore. Other than that it seems to work fine so far.

const readable = new Readable({objectMode: true}).wrap(pool.query(sql).stream());
for await (const result of readable) {
  if (result.constructor.name !== 'OkPacket') {
    console.log(result);
  }
}