fix: only set readableObjectMode in recognize-stream if not present

[MasterOdin]

Checklist

• npm test passes (tip: npm run autofix can correct most style issues)
• tests are included
• documentation is changed or added

This fixes a bug in lib/recognize-stream.ts where on Node 12 using it would throw the following error:

TypeError: Cannot set property readableObjectMode of #<Readable> which has only a getter
    at new RecognizeStream (./node_modules/ibm-watson/lib/recognize-stream.js:126:38)

This is because Node 12.3 added a readableObjectMode getter to the Readable stream prototype. This patch changes it such that it now explicitly sets the readableObjectMode option before passing it to the Duplex constructor (which in turn gets passed to the Readable constructor).

It should be noted that on Node 10 and below (which is how it currently works), this.readableObjectMode can be true / undefined while on Node 12, this.readableObjectMode can be true / false. I'm not sure if you folks would want to break backwards compatibility (slightly) and make this.readableObjectMode be true / false to match the new Node 12 signature.

I tested these changes locally in a project I'm working on. No tests were added/changed as no unit tests for the class exists, and I guess it's handled indirectly through the integration tests, but I'm not sure how to set that all up.

[codecov-io]

Codecov Report

Merging #907 into master will decrease coverage by 27.69%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master    #907      +/-   ##
=========================================
- Coverage      54%   26.3%   -27.7%     
=========================================
  Files          18      18              
  Lines        4464    4466       +2     
  Branches      906     907       +1     
=========================================
- Hits         2411    1175    -1236     
- Misses       2051    3290    +1239     
+ Partials        2       1       -1

Impacted Files	Coverage Δ
lib/recognize-stream.ts	14.45% <0%> (-50.79%)	⬇️
lib/common.ts	30% <0%> (-70%)	⬇️
assistant/v2.ts	20.33% <0%> (-62.72%)	⬇️
test/resources/service_error_util.js	20% <0%> (-60%)	⬇️
lib/synthesize-stream.ts	25% <0%> (-60%)	⬇️
assistant/v1.ts	19.29% <0%> (-56.78%)	⬇️
text-to-speech/v1-generated.ts	42.17% <0%> (-36.6%)	⬇️
language-translator/v3.ts	30.56% <0%> (-29.26%)	⬇️
tone-analyzer/v3.ts	61.17% <0%> (-28.24%)	⬇️
... and 9 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1eb8080...86a9d83. Read the comment docs.

[MasterOdin]

Not sure why the integration tests didn't run (which is probably why the code coverage is reported as tanking so much) and not sure if there's anything I can do to get them running on Travis?

[dpopp07]

@MasterOdin Thanks for the PR!

About the integration tests, yeah I was just noticing that... That is certainly why the coverage is reporting so low, I do not expect that drop when merged 🙂

The credentials are encrypted and shipped with the repo so I don't know why they wouldn't have run. I'll look into that.

I will review this today 👍

[MasterOdin]

Unfortunately, due to the need for encryption of credentials, it's impossible to have that work for PRs from forks (which makes sense as it prevents me from revealing them and stealing them).

It might be helpful to write some words on how to set things up in tests/readme.md, especially the services that require setting them up in some way (e.g Watson Assistant, Discovery)

[dpopp07]

This looks good! Thanks for the change

[dpopp07]

That's a good idea, I'll look into that. As far as this:

It should be noted that on Node 10 and below (which is how it currently works), this.readableObjectMode can be true / undefined while on Node 12, this.readableObjectMode can be true / false. I'm not sure if you folks would want to break backwards compatibility (slightly) and make this.readableObjectMode be true / false to match the new Node 12 signature.

What would be an example of a user's code that would be rendered incompatible by this change? I think we've accepted readableObjectMode: false and that hasn't caused any issues.

[MasterOdin]

Yeah, I think I was a bit ambiguous on what I mean. The change doesn't affect what parameters are accepted by the function, but rather just what readableObjectMode field might return on an instantiated RecognizeStream object. This has no effect on how the object itself will function internally. It may be good though to explicitly set readableObjectMode property to false on Node 10 to help prepare for Node 12 transition, though if going by strict semvar would I think require holding off till v5 release.

For example:

let a = new RecognizeStream({});  // or {readableObjectMode: false}
console.log(a.readableObjectMode);

On Node 10, this will print undefined and on Node 12, this will print false, as currently this line:

node-sdk/lib/recognize-stream.ts

Line 139 in 1eb8080

this.readableObjectMode = true;

is the only line in which that property gets set on Node 10.

[dpopp07]

@MasterOdin Okay, that makes sense - thanks. I think we hold off for now and I merge this PR as is.

Feel free to open an issue if you think we should change this in v5. Note though that we will still be supporting Node 10 in v5 so we will need a solution that is compatible with both.

[watson-github-bot]

🎉 This PR is included in version 4.2.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀