Skip to content

feat(microservices): add type for rmq connection options#13105

Merged
kamilmysliwiec merged 1 commit into
nestjs:masterfrom
Deniks:deniks-rmq-options-fix
Feb 7, 2024
Merged

feat(microservices): add type for rmq connection options#13105
kamilmysliwiec merged 1 commit into
nestjs:masterfrom
Deniks:deniks-rmq-options-fix

Conversation

@Deniks

@Deniks Deniks commented Jan 27, 2024

Copy link
Copy Markdown
Contributor

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?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Currently in RabbitMq client options socketOptions.connectionOptions type is any
Issue Number: #13071

What is the new behavior?

  • RabbitMq client options socketOptions.connectionOptions type is now AmqpConnectionOptions
  • In server-rmq.ts and client-rmq.ts local variable rqmPackage has been renamed to rmqPackage

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@coveralls

coveralls commented Jan 27, 2024

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 99c0b257-fa5c-4adb-8791-cd5ea5b9e55c

  • -2 of 6 (66.67%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.197%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/microservices/client/client-rmq.ts 2 3 66.67%
packages/microservices/server/server-rmq.ts 2 3 66.67%
Totals Coverage Status
Change from base Build 889ffab4-7a4b-4e81-8f8a-630e303f4274: 0.0%
Covered Lines: 6723
Relevant Lines: 7292

💛 - Coveralls

heartbeatIntervalInSeconds?: number;
findServers?: () => string | string[];
connectionOptions?: any;
connectionOptions?: AmqpConnectionManager['connectionOptions'];

Copy link
Copy Markdown
Member

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

@Deniks Deniks Jan 29, 2024

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Member

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
@Deniks
Deniks force-pushed the deniks-rmq-options-fix branch from d637bd4 to 4008cc2 Compare January 31, 2024 23:04
findServers?: () => string | string[];
connectionOptions?: any;
connectionOptions?: AmqpConnectionOptions;
clientProperties?: ClientProperties;

@Deniks Deniks Jan 31, 2024

Copy link
Copy Markdown
Contributor Author

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?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add RabbitMQ connection options

3 participants