search

Azure / ms-rest-nodeauth

node.js based authentication library for Azure with type definitions
MIT License
33 stars 33 forks source link

Stengthen types around AuthResponse #121

Closed ramya-rao-a closed 3 years ago

ramya-rao-a commented 3 years ago

This PR strengthens the types around AuthReponse inspired by the changes in #75 Creating this PR as we did not get a response to https://github.com/Azure/ms-rest-nodeauth/pull/75#issuecomment-770492773

Additionally, this updates the execAz command to return a rejected promise when there is no output from running the az command

arthurschreiber commented 3 years ago

@ramya-rao-a Ah, sorry - I totally missed your message over in #75!

You might have to release these changes as semver-major, if the version numbers of this package are following semantic versioning. This is because I think some of these type changes are breaking backwards compatibility (but they're more correct than before).

Specifically, the change to the return type of withAuthFile() does not seem to be backwards compatible.

ramya-rao-a commented 3 years ago

Hey @arthurschreiber

Can you elaborate on why the change in the return type for withAuthFile() would be breaking while similar changes to the return types for other method would not be?

Previously, the return type would be around TokenCredentialsBase and now it would be ApplicationTokenCredentials | ApplicationTokenCertificateCredentials. Both ApplicationTokenCredentials and ApplicationTokenCertificateCredentials extend TokenCredentialsBase.

If we were changing the return type from say ApplicationTokenCredentials to ApplicationTokenCredentials | ApplicationTokenCertificateCredentials, then I can see how the change would be breaking because not all properties of ApplicationTokenCredentials are available in ApplicationTokenCertificateCredentials

@sadasant, I see you have upvoted Arthur's concerns above. Do you have any insights on how the changes to the withAuthFile is breaking?

sadasant commented 3 years ago

@ramya-rao-a no, I think you’re correct, I just wanted to validate the feedback somehow. Feedback is always good!