feat: remove publish generic

[y-nk]

The current definition does not bring any value as the generic type T is not used anywhere in the code, but it takes over any attempt to manually override the type of AmpqConnection with an ambient definition such as:

type Events = {
    'event1': { foo: string }
    'event2': { bar: number }
}

declare module '@golevelup/nestjs-rabbitmq' {
    interface AmqpConnection {
        publish<T extends keyof Events>(
            exchange: 'my-exchange',
            routingKey: T,
            message: Events[T],
            opts: Options.Publish
        ): Promise<boolean>
    }
}

demo of the type issue here: https://www.typescriptlang.org/play/?#code/JYWwDg9gTgLgBDAnmApnA3nA8mGwIB2AznAL5wBmUEIcA5AIYgCOYANsAEZ0BQoksDHACCLMAGFCBFAGM8hMpWq06AAQDmENigBuKNgFcwAemlEYAKyIBaKA06dgMFrx5JUcAKJ6CMEgF4MHjgQ+l0UXwBGOgAuIQoICDjzKGACdTJg0Lpw3wAmWKFOBig4ggMQThQoTNIeHgATWTYStBAIBoNteg0tcMMTM0sbOwcnFyDQuDSYaooGGTRRVkkCaTl8Akmp0LADTg4iAAsAHgAVOBQAD1mCBpIAaxRECAovHz8APgAKLJ2d64yI4MdIoOJ0ECIayA4GgugAGj+-1C1AMeHSAGlnnEzojkTsQCgiEQGOowe8In4ANpnAC6ePxoQguCIcRw8mIADoAAr7Q5HJFTACUcW5ymARBQJ04iW0IM+SLqdR4MkI5jgqrWcEC0gA7iIxKt1hzvpgDKlwXQyEL6sZjAgjhLpiRUFAQE5Zg04FUZAwDJLpvB5mw2CQZhAHWhIur+NpCb4GBy4LrHUC4AwQxBdSRCcTSWgYBGqnALoEQYgVVJOXsDhKjt8IVCYSCyQiwh9ovD6AQIDBrAkINYUml1HQbTw7dM3i8DAgoIh02iIKrwNoOV2nMnoA8SOooChE3ADL5gGw54gRwgI2T4EQDOoyeZNiRXpG4LmSWSEMgUBP7YeYEdHMaEpLtALQABJAARTw4CaGRgCaEgqhgXUUAiClfCIGlaTgb48juOCUAoNInE2IV00I8s8OjeAmhIggyMICiQQaP8NRBHt4DAagdEQtA-ULawV3YFAkyINJFkDDU1X4qASEAgCjjQAivXo0ikydLj30TIEUC9Tg0UjdjaLw8sKKdCAHk5Ss1mrPk6wbSFoSuIEWxQNscg7NtMAHOJImtIA

Therefore, i would like to remove it.

[underfisk]

@y-nk I do understand where your coming and the reproduction but rather than striping the generic, would it be valuable to improve the typings instead of going back to any ?
Your approach of augmenting the module works outside but what if we had a type-safe approach to help resolving events/payloads?

[y-nk]

@underfisk i would really like to do this within the package itself, i went for type augmentation with fear of rejection for such a proposal :)

i'm still trying to figure out the best way, type augmentation has good pros and adding generic has cons. i think it could be nice if the AmqpConnection class would allow a <T> directly (as opposed to being at .publish level) but it would need some kind of convention.

The reason for a convention (rather than type enforcement) is that any attempt to type the said convention would end up in smoothing the types too much and loosing unions which are the main benefit for such a DX: if we did something like Record<ExchangeName, Record<RoutingKey, Message>> then the consequence would be that type definition would end up dealing with 2 levels of "melted" unions (rather than A[B][C]).

To give you some idea of where i am, I have built this (semi-successfully since i had to alter AmqpConnection .d.ts in node_modules/):

type AggregatedEvents = {
  "event-bus": {
    "event1": { foo: string },
    "event2": { bar: number },
  }
}

type Exchange = keyof AggregatedEvents

type RoutingKeys<T extends Exchange> = keyof AggregatedEvents[T]

type Message<E extends Exchange, R extends RoutingKeys<E>> = AggregatedEvents[E][R]

declare module '@golevelup/nestjs-rabbitmq' {
  interface AmqpConnection {
    publish<
      E extends Exchange,
      R extends RoutingKeys<E>,
      M extends Message<E, R>,
    >(exchange: E, routingKey: R, message: M, options?: Options.Publish): Promise<boolean>;
  }
}

The real content of AggregatedEvents is rather a semi-manual computed type which is built out of our fleet of microservices (gathered automatically thanks to a pnpm monorepo) ; this ↑ is distributed as a "client" package for service to service "typed" comms.

I don't know if such a type is nice to enforce, especially that it would mean you need to import your AmqpDefinition type everywhere. the beauty of type augmentation (and why i went this way) is that as soon as you import the .d.ts file, wherever there's a AqmpConnection class, it would be typed.

[underfisk]

@y-nk I understand your point and at some degree I think that is definitely better than the current generic we have. My main fear with removing this is that it may break usages of people utilizing the generic 😅 (it would probably imply highlighting this as a breaking change)

If we were do define a placeholder type, you could override with type augmentation instead of the generic but the generic would have to exit but assigned by default to that type. I could try to write something on later (don't have much time) to illustrate.

If someone else agrees on removing the generic as it may not be used and release with that highlighted, I'm okay on merging this but until then figuring out a better solution would be ideal

[underfisk]

@underfisk i would really like to do this within the package itself, i went for type augmentation with fear of rejection for such a proposal :)

i'm still trying to figure out the best way, type augmentation has good pros and adding generic has cons. i think it could be nice if the AmqpConnection class would allow a <T> directly (as opposed to being at .publish level) but it would need some kind of convention.

The reason for a convention (rather than type enforcement) is that any attempt to type the said convention would end up in smoothing the types too much and loosing unions which are the main benefit for such a DX: if we did something like Record<ExchangeName, Record<RoutingKey, Message>> then the consequence would be that type definition would end up dealing with 2 levels of "melted" unions (rather than A[B][C]).

To give you some idea of where i am, I have built this (semi-successfully since i had to alter AmqpConnection .d.ts in node_modules/):

type AggregatedEvents = {
  "event-bus": {
    "event1": { foo: string },
    "event2": { bar: number },
  }
}

type Exchange = keyof AggregatedEvents

type RoutingKeys<T extends Exchange> = keyof AggregatedEvents[T]

type Message<E extends Exchange, R extends RoutingKeys<E>> = AggregatedEvents[E][R]

declare module '@golevelup/nestjs-rabbitmq' {
  interface AmqpConnection {
    publish<
      E extends Exchange,
      R extends RoutingKeys<E>,
      M extends Message<E, R>,
    >(exchange: E, routingKey: R, message: M, options?: Options.Publish): Promise<boolean>;
  }
}

The real content of AggregatedEvents is rather a semi-manual computed type which is built out of our fleet of microservices (gathered automatically thanks to a pnpm monorepo) ; this ↑ is distributed as a "client" package for service to service "typed" comms.

I don't know if such a type is nice to enforce, especially that it would mean you need to import your AmqpDefinition type everywhere. the beauty of type augmentation (and why i went this way) is that as soon as you import the .d.ts file, wherever there's a AqmpConnection class, it would be typed.

You could also have your own client and re-use across your microservices and that way the client is type-safe (this is an alternative to the type augmentation). Imagine you're using Zod (or any other library) to have type-safety but also runtime validations. A client that is able to consume the payload schema and provide its types to the consumer will be able to achieve the same.

Do you think you can help us out finding a middle ground that allows the package to provide either type augmentation with the least breaking change or the package providing its own generic that supports an easy override/extension?

[y-nk]

of course i'm willing to help 😬 i'm a big user of the package and i do like it. it'd be great!

i think we can work a middle ground but i'm thinking that (i need to test this) the current definition (T = any) will probably take over if there are any type issues as it does currently with my augmentation, that is because that signature act as a trap for when a stronger type is wrong, it would fall into "oh no pb then, we can use T = any then".

on the possible break of the usage of that T generic, it is indeed possible that it is used by people but their usage would have to be along the line of:

amqpConnection.publish<SomePayload>("", "some-event", somePayload)

...because otherwise, since T can be anything, writing just amqpConnection.publish("", "some-event", somePayload) would still work exactly the same (T would be implicitly defined as SomePayload) and most importantly, that T generic is not used at all in the publish function!

[y-nk]

You could also have your own client and re-use across your microservices and that way the client is type-safe (this is an alternative to the type augmentation).

this is another possibility but it would require building a boilerplate which we need to install and integrate from service to service, which is added maintenance just for the sake of type safety.

the said package would need:

• a main module to setup the RabbitMQModule
• a config module to read uri from env
• a service which would only contain the publish function as override just for types

we studied this road and that's why i went for type augmentation instead, it's much lighter and better integration.

[underfisk]

You could also have your own client and re-use across your microservices and that way the client is type-safe (this is an alternative to the type augmentation).

this is another possibility but it would require building a boilerplate which we need to install and integrate from service to service, which is added maintenance just for the sake of type safety.

the said package would need:

• a main module to setup the RabbitMQModule
• a config module to read uri from env
• a service which would only contain the publish function as override just for types

we studied this road and that's why i went for type augmentation instead, it's much lighter and better integration.

Fair enough. We can get started by removing the generic and later on improve it for sure

[lookto]

I understand the intention behind this pr, but removing this introduced a breaking change. This should not have been released in a minor version.