-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[core-amqp] Fixing typos #8956
[core-amqp] Fixing typos #8956
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,29 +25,29 @@ import { RequestResponseLink } from "./requestResponseLink"; | |
export interface CbsResponse { | ||
correlationId: string; | ||
statusCode: string; | ||
satusDescription: string; | ||
statusDescription: string; | ||
} | ||
|
||
/** | ||
* @class CbsClient | ||
* Describes the EventHub/ServiceBus Cbs client that talks to the $cbs endopint over AMQP connection. | ||
* Describes the EventHub/ServiceBus Cbs client that talks to the $cbs endpoint over AMQP connection. | ||
*/ | ||
export class CbsClient { | ||
/** | ||
* @property {string} endpoint CBS endpoint - "$cbs" | ||
*/ | ||
readonly endpoint: string = Constants.cbsEndpoint; | ||
/** | ||
* @property {string} replyTo CBS replyTo - The reciever link name that the service should reply to. | ||
* @property {string} replyTo CBS replyTo - The receiver link name that the service should reply to. | ||
*/ | ||
readonly replyTo: string = `${Constants.cbsReplyTo}-${generate_uuid()}`; | ||
/** | ||
* @property {string} cbsLock The unqiue lock name per $cbs session per connection that is used to | ||
* acquire the lock for establishing a cbs session if one does not exist for an aqmp connection. | ||
* @property {string} cbsLock The unique lock name per $cbs session per connection that is used to | ||
* acquire the lock for establishing a cbs session if one does not exist for an amqp connection. | ||
*/ | ||
readonly cbsLock: string = `${Constants.negotiateCbsKey}-${generate_uuid()}`; | ||
/** | ||
* @property {string} connectionLock The unqiue lock name per connection that is used to | ||
* @property {string} connectionLock The unique lock name per connection that is used to | ||
* acquire the lock for establishing an amqp connection if one does not exist. | ||
*/ | ||
readonly connectionLock: string; | ||
|
@@ -63,7 +63,7 @@ export class CbsClient { | |
|
||
/** | ||
* @constructor | ||
* @param {Connection} connection The AMQP conection. | ||
* @param {Connection} connection The AMQP connection. | ||
* @param {string} connectionLock A unique string (usually a guid) per connection. | ||
*/ | ||
constructor(connection: Connection, connectionLock: string) { | ||
|
@@ -144,7 +144,7 @@ export class CbsClient { | |
} catch (err) { | ||
err = translate(err); | ||
logger.warning( | ||
"[%s] An error occured while establishing the cbs links: %O", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So here's a funny situation. If a customer sends us logs and we go and search for the message in our code we won't be able to find it anymore. Might not be a big deal as long as we're aware that misspellings have been corrected :) CC: @ramya-rao-a , @chradek , @HarshaNalluru There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this was for |
||
"[%s] An error occurred while establishing the cbs links: %O", | ||
this.connection.id, | ||
err | ||
); | ||
|
@@ -269,7 +269,7 @@ export class CbsClient { | |
const cbsResponse = { | ||
correlationId: msg.correlation_id! as string, | ||
statusCode: msg.application_properties ? msg.application_properties["status-code"] : "", | ||
satusDescription: msg.application_properties | ||
statusDescription: msg.application_properties | ||
? msg.application_properties["status-description"] | ||
: "" | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richardpark-msft I wonder if this could break anything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this would be a breaking change. If we are doing this, then we will have to update the major version to 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gah, the curse of having an "internal" library be externally available.
Everything else in this PR is goodness, let's just leave this change out for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know...I kind of see it as a bug fix. I think the fact that no one has reported that it's typo'd as
satusDescription
is some indicator that probably no one is using this field yet. If we really wanted, we could create an alias in JS so anyone using that field at runtime would still see it. Though having done something like that in the past people will wonder why they both show up...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should fix this - it's not super clear that this is typo'd, I might just not even realize what it is.
There is a hybrid approach here - we can support both, but have the typings (and docs) only show latest. That way JS users don't need to do anything, and TS builds break but the fix is easy - it will offer a "did you mean" error since the string is only off-by-one, and the user could just cast to any without any code changes to make the error go away and things work... or even if they just ignore the error, the emitted code will continue to work also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, what do you all think of this message?
CbsResponse's property "satusDescription" is deprecated, use "statusDescription" instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If today you have:
you would change it to
and make sure you add @deprecated to the field's type definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While taking this approach makes sense for when we want to deprecate properties, there is not much of a return on investment here. There most likely is no user scenario to even expose the statusCode and the statusDescription in this case.
For the scope of this PR, I would recommend not making any change to
satusDescription
. @richardpark-msft, I would recommend we log an issue to investigate the usefulness of the status code & description when getting back the response of a claim negotiation. I am sure that a deeper dive into core-amqp may bring other things we want to change in a v2There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I'm undoing that specific change then.
@bterlson Thank you for that though! I haven't used
defineProperty
in ages 😱 amazing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richardpark-msft Here's an issue! #9031 Good luck 🌞