implement toNodeStream()

[NotBobTheBuilder]

There had been some discussion in #279 - To Nodejs Stream of a toNodeStream method, which I've implemented here.

Inclusion of the stream-browserify package means this should build seamlessly for the purposes of non-node environments and use a shim in those cases.

[vqvu]

I don't think this is necessary. According to the node-browserify npm page, we'll get this automatically when using browserify.

[vqvu]

Should this Readable be marked as objectMode?

[vqvu]

Thanks for this.

I've added a few minor comments. Also, can you add some tests. Doesn't need to be in-depth. Just verify that the output is a Readable and it emits the correct values (including errors).

[NotBobTheBuilder]

Happy to help! I'll put some tests together now.

With regards to the objectMode question, I was originally testing with a Highland wrapper around a file stream, so this code worked as-is. I now realise that object streams wouldn't work.

My first impression was to add a parameter to toNodeStream for specifying custom options, of which {objectMode: true} could be one. that options object would then just be relayed straight into the Readable constructor.

Does that sound like a reasonable strategy?

[NotBobTheBuilder]

For additional context, the reason I didn't just set {objectMode: true} by default is because I was using this method in conjunction with Hapi, which cannot use an object mode stream as a http response

[vqvu]

I think having a options object that's passed directly to the Readable constructor would work. Make sure to add a note to the method docs to remind people that they need to set objectMode to true. Highland streams work with objects, so I can see people assuming that we would set objectMode to true by default.

[NotBobTheBuilder]

That makes sense, thanks. Given there's room for confusion do you think it would be worthwhile to require a choice (possibly with a shorthand for those who don't need to use the full options object).

E.g:

if (options === true) {
  options = {objectMode: true};
} else if (options === false) { 
  options = {objectMode: false};
} else if (!options || (typeof options.objectMode === 'undefined')) {
  throw new Error('toNodeStream requires explicitly specifying objectMode true or false');
}

[vqvu]

I don't think that's necessary. My suspicion is that most uses of this method will want a regular Readable. Highland is probably a better objectMode stream than objectMode Readables are. Plus, there's benefit in having the interface look like the Readable constructor. We just want to remind people in case they *do* want an objectMode Readable. On Mar 10, 2018 11:23 PM, "Jack Wearden" <notifications@github.com> wrote: That makes sense, thanks. Given there's room for confusion do you think it would be worthwhile to require a choice (possibly with a shorthand for those who don't need to use the full options object). E.g: if (options === true) { options = {objectMode: true}; } else if (options === false) { options = {objectMode: false}; } else if (!options || (typeof options.objectMode === 'undefined')) { throw new Error('toNodeStream requires explicitly specifying objectMode true or false'); } — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#644 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGIyZeOJIczn6x_4xmf7XNouWRyZNbZ8ks5tdLS4gaJpZM4SlNWs> .

[NotBobTheBuilder]

Thanks! I've now added tests, docs & the options parameter.

[vqvu]

The build fails on older versions of Node. I know that some of these are pretty old, since we haven't updated the list of versions to test in a while, but can you fix the error? It looks like maybe you just need to use the Buffer constructor directly rather than Buffer.from()?

[vqvu]

Also, can you add an entry to the CHANGELOG.md under 2.12.0?

[vqvu]

.toNodeStream(false) and .toNodeStream(true) are not valid code anymore.

[NotBobTheBuilder]

Thanks - all updated!

[NotBobTheBuilder]

Looks like it still failed in versions 4 & 5, I'll fix that too.

Would it be worth me adding some newer versions also to the Travis build matrix too? It looks like 7 is the most recent specified (& 6 is the most recent executing)

[vqvu]

Node 7 is only on the master branch, which is the test matrix for the 3.x beta. That's why it's not running. I think we should add 8 and 9, remove 0.12 and 5, and change "4.0" to "4", but not in this PR. 0.10 is helpful as a kind of proxy for older browsers, so I'd like to leave it in.

Edit: CL -> PR

[NotBobTheBuilder]

Thanks!

[vqvu]

I've released this change in 2.12.0.