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

Add support for bullmq (bull v4) and some small improvements #49

Merged
merged 3 commits into from
Jan 20, 2020
Merged

Add support for bullmq (bull v4) and some small improvements #49

merged 3 commits into from
Jan 20, 2020

Conversation

Embraser01
Copy link
Contributor

@Embraser01 Embraser01 commented Jan 8, 2020

Hi,

I added support for bull v4 (#38).

  • Now, bull and bullmq are both optional peer dependencies and must be added by the user.
  • The bull version is displayed on the dashboard
  • For now, the createQueues function does not support bull 4

Unrelated (I can move it to another PR if you want), I added a button to toggle data instead of displaying it directly. In case of big data blob, the UI was freezing

closes #32
closes #38

@Embraser01 Embraser01 marked this pull request as ready for review January 9, 2020 09:33
index.js Outdated
})

return queues
},
createQueues: redis => {
if (isBullMQ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been actually considering removing the option to createQueues and keep only setQueues, what do you think?

this way we can keep the lib easier to maintain! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Make sense, I'll remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realize that it was the only place where we needed to require('bull'). It means that we can remove bull and bullmq from peerDependencies, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes! exactly, this way we don't need to control the lib version or anything, leaving it only up to the users and making sure we support the correct versions 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some testing, I saw that some functions have changed signature between Bull3 and BullMQ...

I can detect the version of a queue by checking (for example) if the drain method exists (BullMQ) or not (Bull3). This way, we can switch if needed...

For example, queue.clean has not the same signature:

// With BullMQ
await queue.clean(GRACE_TIME_MS, LIMIT, queueStatus)
// With Bull3
await queue.clean(GRACE_TIME_MS, queueStatus)

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that's tricky 😅
I would say we can have some check when constructing the lib, and have that variable in the locals, I guess it will be a consequence to have some conditions in some places

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a map to store the version for each queue (this way a system can have both versions simultaneously). The function that check the bull version is here

@vcapretz
Copy link
Contributor

vcapretz commented Jan 9, 2020

that is amazing @Embraser01! thank you very much for this PR

I added only one minor comment, but other than that I think it's fine!

But I guess this would be enough for a 1.0 release, and for that I want to improve some other things in the lib, so I'll probably merge soon and hold the release to include more stuff

@eric-hc
Copy link

eric-hc commented Jan 9, 2020

Unsolicited suggestion, but could you add the v4 docs (https://docs.bullmq.io) to the README in this PR? Might be helpful for new users who are starting with v4.

Bull's docs are referenced here

@vcapretz
Copy link
Contributor

vcapretz commented Jan 9, 2020

no problems @ericcarboni!

@Embraser01 can you add it please? at least some basic stuff, I'll make sure the docs are improved as much as possible before bumping to v1.0.0

example.js Outdated Show resolved Hide resolved
example.js Outdated Show resolved Hide resolved
example.js Outdated Show resolved Hide resolved
@vcapretz
Copy link
Contributor

I think this PR is getting in great shape to be merged!

@vcapretz vcapretz merged commit ed9ab9b into felixmosh:master Jan 20, 2020
@Embraser01 Embraser01 deleted the bullmq-compat branch January 20, 2020 10:21
@vcapretz
Copy link
Contributor

thanks for the awesome work @Embraser01!

I have a revamp branch where I'm doing a lot of changes to be able to land a 1.0.0, this will be included there!

@Embraser01 Embraser01 restored the bullmq-compat branch January 20, 2020 10:58
@seawatts
Copy link

Did this get published to NPM?

@vcapretz
Copy link
Contributor

No, not yet 😅 I’m preparing a 1.0 launch, I’ll probably release a beta version so if you would like to try it out please follow the pr #51!

@Embraser01 Embraser01 deleted the bullmq-compat branch February 24, 2020 11:05
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.

support for Bull 4 It doesnt show the job name
5 participants