Skip to content
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

Revamp NodeJS module for better DI and multiple connections #24

Merged
merged 8 commits into from
May 18, 2022

Conversation

EnriqCG
Copy link
Owner

@EnriqCG EnriqCG commented May 17, 2022

Context of the revamp

Previous partial support for handling multiple AMQP connections involved using the same root AMQPModule and passing an array of connections.

static forRoot(options: AMQPModuleOptions | AMQPModuleOptions[]): DynamicModule {

amqplib connection objects were stored within the same module in a Map. When performing actions on a particular connection or channel, retrieval from the Map had to happen:

getChannel(connectionName?: string): ChannelWrapper {
if (!connectionName) {
connectionName = this.amqpClient.defaultKey
}
if (!this.amqpClient.clients.has(connectionName)) {
throw new Error(`client ${connectionName} does not exist`)
}

This deviates from what NestJS does on modules like @nestjs/mongoose or @nestjs/typeorm where multiple connections are handled by different instances of the module. This is how @nestjs/mongoose handles multiple connections:

@Module({
  imports: [
    MongooseModule.forRoot('mongodb://localhost/test', {
      connectionName: 'cats',
    }),
    MongooseModule.forRoot('mongodb://localhost/users', {
      connectionName: 'users',
    }),
  ],
})
export class AppModule {}

And when wanting to use a resource for a module, an Injector is provided to inject the resource for a particular connection:

export class CatsService {
  constructor(@InjectConnection('cats') private connection: Connection) {}
}

This behavior also deviates from the current implementation where no injection happens, and instead, a shared "AMQPService" is provided to get access to channels. Not doing dependency injection right has implications for testing and goes against core design principles for NestJS.

(Breaking) changes made to fix this behavior

(not a comprehensive list)

  • AMQPModule.forRoot now ONLY accepts a single connection object instead of an array of connections. The first is for connection details.
forRoot(connectionOptions: AMQPModuleOptions)
  • The library now creates two separate channels. One is used for publishing and exported for any module to use, and the other one used to consume messages (used when using the @Consume decorator).
  • New channel and connection token getters for handling multiple connections.
  • Exported injectors so a connection/channel can be injected to any class.
    • Removed AMQPService in favor of injectors.
export class JobsService {
  constructor(
    @InjectAMQPChannel('connectionName!')
    private amqpChannel: Channel,
  ) {}
  • Removed the AMQPCoreModule. Now, all logic is in the AMQPModule.

Closes #3.

@EnriqCG EnriqCG merged commit 3cfbae6 into main May 18, 2022
@EnriqCG EnriqCG deleted the revamp-connections branch May 18, 2022 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple AMQP connections
1 participant