Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make RecognizeStream.readableObjectMode always return Boolean #943

Merged
merged 1 commit into from
Aug 26, 2019

Conversation

MasterOdin
Copy link
Contributor

Unifies the return type of RecognizeStream.readableObjectMode to always return Boolean instead of Boolean on Node 12+ and true | undefined for Node 10-. While this is a very slight break in behavior, figured it would be better for v5 than pushing to v4 just to be safe. See #907 for discussion on this, namely the final two comments: #907 (comment) and #907 (comment).

Checklist
  • npm test passes (tip: npm run autofix can correct most style issues)

@codecov-io
Copy link

Codecov Report

Merging #943 into release-candidate-v5 will decrease coverage by 27.43%.
The diff coverage is 0%.

Impacted file tree graph

@@                    Coverage Diff                    @@
##           release-candidate-v5     #943       +/-   ##
=========================================================
- Coverage                 54.36%   26.92%   -27.44%     
=========================================================
  Files                        18       18               
  Lines                      4505     4505               
  Branches                    906      906               
=========================================================
- Hits                       2449     1213     -1236     
- Misses                     2054     3291     +1237     
+ Partials                      2        1        -1
Impacted Files Coverage Δ
lib/recognize-stream.ts 14.63% <0%> (-50.01%) ⬇️
lib/common.ts 23.07% <0%> (-69.24%) ⬇️
assistant/v2.ts 20.33% <0%> (-62.72%) ⬇️
test/resources/service_error_util.js 20% <0%> (-60%) ⬇️
lib/synthesize-stream.ts 25.86% <0%> (-56.9%) ⬇️
assistant/v1.ts 19.29% <0%> (-56.78%) ⬇️
text-to-speech/v1-generated.ts 47.85% <0%> (-33%) ⬇️
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 5c11c85...a7aa0e4. Read the comment docs.

1 similar comment
@codecov-io
Copy link

codecov-io commented Aug 24, 2019

Codecov Report

Merging #943 into release-candidate-v5 will decrease coverage by 27.43%.
The diff coverage is 0%.

Impacted file tree graph

@@                    Coverage Diff                    @@
##           release-candidate-v5     #943       +/-   ##
=========================================================
- Coverage                 54.36%   26.92%   -27.44%     
=========================================================
  Files                        18       18               
  Lines                      4505     4505               
  Branches                    906      906               
=========================================================
- Hits                       2449     1213     -1236     
- Misses                     2054     3291     +1237     
+ Partials                      2        1        -1
Impacted Files Coverage Δ
lib/recognize-stream.ts 14.63% <0%> (-50.01%) ⬇️
lib/common.ts 23.07% <0%> (-69.24%) ⬇️
assistant/v2.ts 20.33% <0%> (-62.72%) ⬇️
test/resources/service_error_util.js 20% <0%> (-60%) ⬇️
lib/synthesize-stream.ts 25.86% <0%> (-56.9%) ⬇️
assistant/v1.ts 19.29% <0%> (-56.78%) ⬇️
text-to-speech/v1-generated.ts 47.85% <0%> (-33%) ⬇️
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 5c11c85...a7aa0e4. Read the comment docs.

Copy link
Contributor

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, thanks! Just to confirm my understanding, this changes what would be the default behavior for Node 10, but it does not prevent this from functioning with Node 10, right?

@dpopp07 dpopp07 merged commit de9fe5a into watson-developer-cloud:release-candidate-v5 Aug 26, 2019
@MasterOdin
Copy link
Contributor Author

MasterOdin commented Aug 26, 2019

Yeah, it just unifies what calling this property in the case that you don't set it to true will return such that node 10 will return the same thing as node 12 (false).

Example code:

const RecognizeStream = require('./lib/recognize-stream.js');

let stream = new RecognizeStream({});
console.log(stream.readableObjectMode);

stream = new RecognizeStream({objectMode: true});
console.log(stream.readableObjectMode);

Environment:

$ node --version
v12.9.0
$ /usr/local/opt/node@10/bin/node --version
v10.16.3

Before patch:

$ node test.js
false
true
$ /usr/local/opt/node@10/bin/node test.js
undefined
true

After patch:

$ node test.js
false
true
$ /usr/local/opt/node@10/bin/node test.js
false
true

Like I mentioned in the PR, this is a fairly inconsequential BC break that it could probably be safe to push to v4, but given that there's an upcoming v5 soon, might as well do it "right" per semver guidelines.

dpopp07 pushed a commit that referenced this pull request Sep 19, 2019
)

BREAKING CHANGE: `RecognizeStream.readableObjectMode` will always be a Boolean value - before, it could have been `undefined`.
@MasterOdin MasterOdin deleted the patch-3 branch September 27, 2019 12:00
dpopp07 pushed a commit that referenced this pull request Oct 4, 2019
)

BREAKING CHANGE: `RecognizeStream.readableObjectMode` will always be a Boolean value - before, it could have been `undefined`.
watson-github-bot pushed a commit that referenced this pull request Oct 4, 2019
# [5.0.0](v4.5.1...v5.0.0) (2019-10-04)

### Bug Fixes

* make RecognizeStream.readableObjectMode always return Boolean ([#943](#943)) ([a276df4](a276df4))

### Build System

* drop support for Node versions 6 and 8 ([3ea1fd7](3ea1fd7))

### Code Refactoring

* change all websocket method parameters to lower camel case ([#941](#941)) ([cb6711f](cb6711f))

### Features

* add support for new authenticators in all sdks and add new service features for major release ([#946](#946)) ([3acffc5](3acffc5))

### BREAKING CHANGES

* Passing individual credentials to the service constructor will no longer work. An Authenticator must be initialized and passed in. For more information, see the migration guide.
* All parameters have been converted to their lower camel case version.
* Support for the `token` parameter has been removed
* Support for the `customization_id` parameter has been removed
* Method `setAuthorizationHeaderToken` has been removed from the WebSocket Stream classes
* `RecognizeStream.readableObjectMode` will always be a Boolean value - before, it could have been `undefined`.
* This SDK may no longer work with applications running on Node 6 or 8.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants