Skip to content

Conversation

dpopp07
Copy link
Contributor

@dpopp07 dpopp07 commented Sep 23, 2019

Export this type to be used in the generated subclasses.

@codecov
Copy link

codecov bot commented Sep 23, 2019

Codecov Report

Merging #50 into release-v1-rc2 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           release-v1-rc2      #50   +/-   ##
===============================================
  Coverage           95.71%   95.71%           
===============================================
  Files                  23       23           
  Lines                 584      584           
  Branches              125      125           
===============================================
  Hits                  559      559           
  Misses                 24       24           
  Partials                1        1
Impacted Files Coverage Δ
lib/base_service.ts 100% <ø> (ø) ⬆️

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 adaae5e...127d101. Read the comment docs.

*/

export { BaseService } from './lib/base_service';
export { BaseService, UserOptions } from './lib/base_service';
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are going to export UserOptions, should you not also export HeaderOptions?

Copy link
Contributor

@mkistler mkistler left a comment

Choose a reason for hiding this comment

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

If we are going to export UserOptions, I think we need to add comments on all the fields of this struct, since it is now an "external interface".

@mkistler
Copy link
Contributor

One more comment related to HeaderOptions. We might want to take this opportunity to redefine it as OutgoingHttpHeaders, to make it consistent with the Node SDKs. Then we'll need to move the handling of X-Watson-Learning-Opt-Out to the Watson SDKs, but I think that's appropriate anyway.

@dpopp07
Copy link
Contributor Author

dpopp07 commented Sep 24, 2019

That's a good point. I actually don't see HeaderOptions defined in the code anywhere. Only in the Typedoc comments, where it definitely needs to be changed. We can also move the documentation of the learning opt out header to Watson, although we would need a way to represent that in the API definition.

I'll go ahead and remove that from the core for now. I'll also add comments for all of the interface fields 👍

@dpopp07 dpopp07 force-pushed the dp/export-user-options-interface branch from 69ff49d to 127d101 Compare September 24, 2019 16:43
@dpopp07
Copy link
Contributor Author

dpopp07 commented Sep 24, 2019

@mkistler Comments addressed. I also re-ordered the properties of the interface

Copy link
Contributor

@mkistler mkistler left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@dpopp07 dpopp07 merged commit 8dcde61 into release-v1-rc2 Sep 24, 2019
@dpopp07 dpopp07 deleted the dp/export-user-options-interface branch September 24, 2019 18:13
ibm-devx-automation pushed a commit that referenced this pull request Oct 3, 2019
# [1.0.0](v0.3.6...v1.0.0) (2019-10-03)

### Bug Fixes

* Move check for serviceUrl to createRequest ([#47](#47)) ([6f04739](6f04739))
* parse result from response in token managers ([6bbe423](6bbe423))
* provide bundlers alternate file for browser support ([#58](#58)) ([88a9d16](88a9d16))

### Build System

* drop support for Node versions 6 and 8 ([#33](#33)) ([d47c737](d47c737))

### Code Refactoring

* look for credentials file in working dir before home dir ([#46](#46)) ([c5556de](c5556de))
* return detailed response as second callback argument ([#34](#34)) ([dc24154](dc24154))

### Features

* add `setServiceUrl` method as a setter for the `serviceUrl` property ([#41](#41)) ([cfb188f](cfb188f))
* add specific error handling for SSL errors with cloud private instances ([#54](#54)) ([056ec9a](056ec9a))
* export `UserOptions` interface from the BaseService ([#50](#50)) ([4f0075a](4f0075a))
* implement new authenticators to handle sdk authentication ([#37](#37)) ([f876b6d](f876b6d))
* refactor core to use Promises instead of callbacks ([#55](#55)) ([9ec8afd](9ec8afd))

### BREAKING CHANGES

* None of the authenticators or request methods take callbacks as arguments anymore - they return Promises instead.
* Users that have credential files in both the working directory and the home directory will see a change in which one is used.
* The internal property `url` no longer exists on the `baseOptions` object, it has been renamed to `serviceUrl`
* The old style of passing credentials to the base service will no longer work. An Authenticator instance MUST be passed in to the base service constructor.
* token managers no longer support user access tokens. use BearerTokenAuthenticator instead
* The class names of the token managers have changed.
* `Icp4dTokenManagerV1` renamed to `Cp4dTokenManager`
* `IamTokenManagerV1` renamed to `IamTokenManager`
* `JwtTokenManagerV1` renamed to `JwtTokenManager`
* The public method `setAuthorizationInfo` is renamed to `setClientIdAndSecret`
* The response body is no longer the 2nd callback argument, the detailed response is. The body is located under the `result` property. The `data` property is removed.
* 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants