-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
feat(microservices): add type for rmq connection options #13105
Conversation
Pull Request Test Coverage Report for Build 99c0b257-fa5c-4adb-8791-cd5ea5b9e55c
💛 - Coveralls |
@@ -25,7 +27,7 @@ export interface AmqpConnectionManagerSocketOptions { | |||
reconnectTimeInSeconds?: number; | |||
heartbeatIntervalInSeconds?: number; | |||
findServers?: () => string | string[]; | |||
connectionOptions?: any; | |||
connectionOptions?: AmqpConnectionManager['connectionOptions']; |
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.
we can't directly reference types from the amqp-connection-manager package as this would lead to compilation errors for those who have "skipLibCheck" set to false in their TS configuration; that's why we have the external directory with copied interfaces
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.
Understood. Was there a reason for setting connectionOptions
to any
and not further copy AmqpConnectionOptions
?
If not, I can similiarly copy the interface. As per #13071, copying just AmqpConnectionOptions.credentials
field technically will suffice and allow to specify External auth, or should I specify whole interface? I think whole interface because ts
will complain if user wants to specify other field than credentials
.
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 can't recall any specific reason so feel free to copy it over
change socketOptions.connectionOptions type from any to copied amqp-manager's AmqpConnectionOptions Closes nestjs#13071
d637bd4
to
4008cc2
Compare
/** | ||
* @publicApi | ||
*/ | ||
export interface AmqpConnectionManagerSocketOptions { | ||
reconnectTimeInSeconds?: number; | ||
heartbeatIntervalInSeconds?: number; | ||
findServers?: () => string | string[]; | ||
connectionOptions?: any; | ||
connectionOptions?: AmqpConnectionOptions; | ||
clientProperties?: ClientProperties; |
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.
a bit wierd that clientProperties
in original amqp manager is inside connectionOptions
.
I suspect interfaces were not updated when amqp-connection-manager version got changed?
change socketOptions.connectionOptions type from any to amqp-manager's AmqpConnectionOptions which allows to specify not only mTLS but also External auth
Closes #13071
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently in RabbitMq client options
socketOptions.connectionOptions
type isany
Issue Number: #13071
What is the new behavior?
socketOptions.connectionOptions
type is nowAmqpConnectionOptions
server-rmq.ts
andclient-rmq.ts
local variablerqmPackage
has been renamed tormqPackage
Does this PR introduce a breaking change?
Other information