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

refactor: add typing to authorization/v1 #910

Merged
merged 1 commit into from
Jul 15, 2019
Merged

refactor: add typing to authorization/v1 #910

merged 1 commit into from
Jul 15, 2019

Conversation

MasterOdin
Copy link
Contributor

Adds typing information to authorization/v1. No functionality in the class was changed.

Some notes (possible changes I think for v5):

  • Had to disable a tslint rule for the existing class variable target_url. Didn't want to change it to pass tslint and break backwards compatibility.
  • Should the callback be mandatory for getToken? Currently, it's possible to do auth.getToken({url: auth.target_url}); which will cause the following error to be shown (if using the apikey method at least):
(node:30952) UnhandledPromiseRejectionWarning: TypeError: cb is not a function
    at /Users/mpeveler/Work/Github/node-sdk/node_modules/ibm-cloud-sdk-core/auth/jwt-token-manager-v1.js:77:24
    at /Users/mpeveler/Work/Github/node-sdk/node_modules/ibm-cloud-sdk-core/lib/requestwrapper.js:206:13
    at processTicksAndRejections (internal/process/task_queues.js:82:5)
(node:30952) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 2)
(node:30952) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

which I'd find confusing. Given that it doesn't make sense to not use a callback (or else you don't get the generated token to use for the service), it should probably be mandatory?

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

@MasterOdin MasterOdin mentioned this pull request Jul 13, 2019
13 tasks
@dpopp07
Copy link
Contributor

dpopp07 commented Jul 15, 2019

Thanks for the PR and the suggestions. I think both of those things would be good changes for v5.

I'll keep those in mind but also, there's a possibility that this module will go away in v5. This would be in favor of just using the token manager directly, since "watson tokens" have pretty much gone away in favor of IAM. We may keep this for convenience though, it's still being discussed. Just a heads up.

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.

Looks good, thanks! 👍

@MasterOdin
Copy link
Contributor Author

MasterOdin commented Jul 15, 2019

That's fine as I don't personally use this module. I just wanted to use this as an initial try at adding more elaborate typing before I tackle the larger/more complex classes/functions that lack typing, and wanted to make sure I wasn't messing things up as I go along.

@dpopp07 dpopp07 merged commit b99c23a into watson-developer-cloud:master Jul 15, 2019
@watson-github-bot
Copy link
Member

🎉 This PR is included in version 4.2.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants