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(connection): require connection to be passed #2335

Merged

Conversation

roggervalf
Copy link
Collaborator

No description provided.

@roggervalf roggervalf changed the base branch from master to feat/better-handling-of-queue-markers December 15, 2023 05:07
@roggervalf roggervalf changed the base branch from feat/better-handling-of-queue-markers to master December 15, 2023 05:08
@roggervalf roggervalf changed the base branch from master to feat/better-handling-of-queue-markers December 15, 2023 05:08
@roggervalf roggervalf merged commit 1867dd1 into feat/better-handling-of-queue-markers Dec 19, 2023
11 checks passed
@roggervalf roggervalf deleted the fix-require-connection branch December 19, 2023 00:22
github-actions bot pushed a commit that referenced this pull request Dec 21, 2023
# [5.0.0](v4.17.0...v5.0.0) (2023-12-21)

### Bug Fixes

* **connection:** require connection to be passed ([#2335](#2335)) ([1867dd1](1867dd1))
* **job:** revert console warn custom job ids when they represent integers ([#2312](#2312)) ([84015ff](84015ff))
* **python:** unify redis connection args for Queue and Worker ([#2282](#2282)) ([8eee20f](8eee20f))
* **worker:** throw error if connection is missing ([6491a18](6491a18))

### Features

* **job:** provide skipAttempt option when manually moving a job ([#2203](#2203)) ([0e88e4f](0e88e4f))
* **python:** use new queue markers ([b0a13e8](b0a13e8))
* **python:** use new queue markers ([4276eb7](4276eb7))
* **worker:** improved markers handling ([73cf5fc](73cf5fc))
* **worker:** improved markers handling ([0bac0fb](0bac0fb))

### BREAKING CHANGES

* **worker:** Markers use now a dedicated key in redis instead of using a special Job ID.
* **worker:** Markers use now a dedicated key in redis instead of using a special Job ID.
@void-nick
Copy link

The connection requirement is not optimized for NestJS integration (should I need to copy the connection for every worker in my app if it's already set in BullModule?)

@leoperria
Copy link

The connection requirement is not optimized for NestJS integration (should I need to copy the connection for every worker in my app if it's already set in BullModule?)

Same here

@rujorgensen
Copy link

The connection requirement is not optimized for NestJS integration (should I need to copy the connection for every worker in my app if it's already set in BullModule?)

Same. What's the recommended course of action here?

@vsamofal
Copy link

same for me :)

@manast
Copy link
Contributor

manast commented Jan 14, 2024

If you do not pass a connection it will use the local connection, so please can you explain how this requirement affects NestJS?

@manast
Copy link
Contributor

manast commented Jan 14, 2024

(the reason for requiring the connection is to avoid bugs where the connection was indadvertedly left undefined so it used the local connection instead in production where there weren't any)

@vsamofal
Copy link

vsamofal commented Jan 14, 2024

NestJS does use types from bullmq, I already made a fix in nestjs nestjs/bull#1960, waiting for PR approval

the problem with nestjs, is that connection type become required and not optional, even tho it works, it breaks code for people and there is no easy way in nestjs to pass a connection because in nestjs these types are used in decorators

@manast
Copy link
Contributor

manast commented Jan 15, 2024

the problem with nestjs, is that connection type become required and not optional, even tho it works, it breaks code for people and there is no easy way in nestjs to pass a connection because in nestjs these types are used in decorators

Yes, that is the reason this was introduced in a major release after more than 6 months of warnings that the option was deprecated.

@void-nick
Copy link

Let me explain the problem a little better: in NestJS, I set up the connection with the forRoot/forRootAsync methods of BullModule, usually in the app top level module. But if I want to use BullMQ version 5 or higher, whenever I need to register a queue, the RegisterQueueOptions interface extends QueueOptions->QueueBaseOptions from BullMQ which will require the connection property again. Since I have already setup in the forRoot method, it doesn't make sense to keep repeating it in every queue registration.
For now, I only install @nestjs/bullmq, which will require BullMQ v4.
It may be just lacking an optional interface for NestJS integration.

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.

6 participants