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

Make mongodb a peer dependency #55

Open
wants to merge 50 commits into
base: main
Choose a base branch
from

Conversation

sebamarynissen
Copy link

@sebamarynissen sebamarynissen commented Feb 1, 2024

This PR is based on my fork and provides support for mongodb@6 by making the mongodb package a peer dependency. Version 4 and 5 of the drivers are still supported too.

One of the benefits of making mongodb a peer dependency is that there is no longer a mismatch between ObjectId of different mongodb versions as agenda will use the ObjectId() from the same mongodb version that you specified during construction. I've experienced that this could sometimes lead to errors.

I've also changed the module to become esm only to prepare for any dependencies becoming esm only one day too.

This was referenced Feb 1, 2024
@Apidcloud
Copy link

Agenda would really benefit from this peer dependency. Atm we have to convert from one ObjectId to another because it's not.

@bompus
Copy link
Contributor

bompus commented Aug 29, 2024

I support this PR over my own ( #46 ), as it ensures there isn't a mismatch between mongodb driver versions.

This not only provides faster start-up times by not loading in two different driver versions, but is likely to eliminate some bugs that could arise from running different versions, such as the ObjectId logic already mentioned.

@Apidcloud
Copy link

Apidcloud commented Oct 2, 2024

MongoDB 5.0 will reach end of life this month (October 2024), and providers like Digital Ocean are already dropping support to 5.0. Any chance we can get this merged asap?

Edit: Any chance you could publish your fork to npm meanwhile, @sebamarynissen ? Ah nvm. Just assumed it wasn't published because of the README, but looked at the package.json and it's available through @whisthub/agenda on npm.

@sebamarynissen
Copy link
Author

My fork is available on npm as @whisthub/agenda. I've been using it in production for a while without any issues.

As for merging this PR, unfortunately I fear that the repo has been effectively abandoned, but my goal is to keep my fork up to date with the latest version of the mongodb driver.

@Apidcloud
Copy link

Ah, yes. I found it a bit ago through the package.json. I'll try to switch to it, then. Thanks @sebamarynissen

@Apidcloud
Copy link

How hard would it be to output commonjs instead of esm @sebamarynissen ? Unfortunately the Digital Ocean thing caught us by surprise and we don't have much time to update our codebases to ESM. I have tried node 22 with --experimental-detect-module and dynamic import your fork, but still couldn't get it to work

@sebamarynissen
Copy link
Author

I think the flag you're looking for is --experimental-require-module, though I would advise to just use a dynamic import. agenda.start() is asynchronous anyway, so normally it shouldn't require a major refactor of your codebase to use a dynamic import in cjs.

@Apidcloud
Copy link

Thanks! That's indeed the flag I was looking for. The dynamic import doesn't seem to work with SWC. I'll give that a go, and report here later

@Apidcloud
Copy link

Apidcloud commented Oct 3, 2024

The flag --experimental-require-module seems to have done the trick with Node 22.

Are you using MongoDb 7 (or the just released v8) in production @sebamarynissen ? We might as well upgrade directly to at least 7 to avoid having to do this again in a few months.

@sebamarynissen
Copy link
Author

sebamarynissen commented Oct 16, 2024

I'm using MongoDB 7 in production, but that shouldn't really matter as long as you use the latest mongodb@6 node.js driver, as it's both compatible with MongoDB 6, 7 and 8, see https://www.mongodb.com/docs/drivers/node/current/compatibility/.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants