-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
feat(microservices): Allow custom exchangeType as string for plugin compatibility #15057
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): Allow custom exchangeType as string for plugin compatibility #15057
Conversation
Pull Request Test Coverage Report for Build db5aa2fa-ca53-4757-a161-3dca69eec471Details
💛 - Coveralls |
5d3ba97
to
ef632bf
Compare
@@ -286,7 +286,7 @@ export interface RmqOptions { | |||
* Type of the exchange | |||
* @default 'topic' | |||
*/ | |||
exchangeType?: 'direct' | 'fanout' | 'topic' | 'headers'; | |||
exchangeType?: string; |
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.
exchangeType?: string; | |
exchangeType?: 'direct' | 'fanout' | 'topic' | 'headers' | string & {}; |
with this (ugly) hack we'd be able to preserve type intelisense
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.
@kamilmysliwiec That would also be good. pragmatic way to preserve Intellisense while keeping flexibility.
Maybe we could define a type and use it explicitly to make things even clearer.
// packages/microservices/external/rmq-url.interface.ts:71
type AmqpMainExchangeType = 'direct' | 'fanout' | 'topic' | 'headers';
/**
* @publicApi
*/
export type AmqpExchangeType = AmqpMainExchangeType | (string & {});
// packages/microservices/interfaces/microservice-configuration.interface.ts:289
exchangeType?: AmqpExchangeType;
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.
external
is supposed to hold files (types) that are copied from the original package, so adding a new custom type there might cause unexpected issues later on
Defined a dedicated type for AMQP exchange types to enhance type safety and maintain IDE support, replacing inline union with a named type for better readability and reusability.
This reverts commit d8f80ef.
@kamilmysliwiec I’ve removed the custom type and updated the code as initially suggested to preserve IntelliSense. |
LGTM |
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, only the default exchange types can be used.
Issue Number: N/A
What is the new behavior?
Make exchangeType configurable as a generic string instead of strict union, enabling support for plugins like rabbitmq-delayed-message-exchange which use non-standard types (e.g.,
x-delayed-message
,x-local-random
).x‑delayed-message
x-delay
header—removes the need for TTL‑DLX stacksx‑consistent-hash
x‑modulus-hash
hash(key) mod N
sharding for linear throughput growthx‑random
x‑local-random
x‑lvc
(Last Value Cache)x‑recent-history
x‑rtopic
(Reverse Topic)*
,#
) while bindings stay static—publisher‑driven multicastx‑management
Does this PR introduce a breaking change?
Other information