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

fix(rabbitmq): using __routeArguments__ key to allow pipe injection #648

Merged

Conversation

sezanzeb
Copy link
Contributor

@sezanzeb sezanzeb commented Sep 22, 2023

Fixes #645

nestjs regular @Param decorator and such adds the pipes to the metadata (via the reflection methods) using the '__routeArguments__' key (https://github.com/nestjs/nest/blob/93b99d09ed84640815fc3aa99196cf94e692bc4d/packages/common/decorators/http/route-params.decorator.ts#L80)

The DependenciesScanner of nestjs then loads the '__routeArguments__' and injects the pipes in reflectParamInjectables (https://github.com/nestjs/nest/blob/master/packages/core/scanner.ts)

golevelup/nestjs however uses the 'RABBIT_ARGS_METADATA' key for it (https://github.com/golevelup/nestjs/blob/c927cb12f5203fe4739027fc54d78c6ae2629cfb/packages/rabbitmq/src/rabbitmq.decorators.ts), so the scanner will never load and inject them.

I just changed it to '__routeArguments__', and then pipes were correctly created, and their dependencies injected. I don't know enough about the internals to judge if this causes other side-effects. Hopefully tests will catch it all. I wasn't able to run tests locally, so I'm waiting for the ci pipeline to run.

@sezanzeb sezanzeb changed the title fix(rabbitmq): using __routeArguments__ to allow pipe injection fix(rabbitmq): using __routeArguments__ key to allow pipe injection Sep 22, 2023
@sezanzeb sezanzeb force-pushed the fix/dependency-injection-for-param-pipes branch from 89f0afe to 637396b Compare September 22, 2023 15:43
@sezanzeb sezanzeb force-pushed the fix/dependency-injection-for-param-pipes branch from 637396b to b1cffbc Compare September 22, 2023 15:43
@underfisk
Copy link
Contributor

Thank you for pushing this PR.
I did think about this before and I believe it was intentional not to have the same key so as not to confuse with controllers. Unfortunately I've been wanting to refactor the package as RabbitMQ providers should have been controllers in the first place but if you're change doesn't introduce any breaking change, I'll be happy to merge

@sezanzeb
Copy link
Contributor Author

At least I haven't observed any problems with this until now. I'll try working with this a bit more over the next one or two weeks, and report back if there have been any unexpected bugs.

@underfisk underfisk merged commit 77b9039 into golevelup:master Sep 27, 2023
3 checks passed
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.

Pipe not working with @RabbitPayload decorator
2 participants