-
Notifications
You must be signed in to change notification settings - Fork 235
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
Mongodb driver 4.x Upgrade #672
Conversation
Thanks for the PR, @arsa-dev! Since this requires an update to This should give us sufficient time to review the PR and do the necessary to prepare a v7.x connector release. |
Okey @achrinza, let me know if I can help in the process to make this merged. I'll be happy to help on this. |
Any news on this? It'll be great to have |
Hi @achrinza, there is any news on this release process? After ~2months of delay on scheduled iteration, our client is requiring us to have official compatibility for mongodb or in his defect to migrate all our projects to another framework/technology that have it. I know that this is only our personal situation but since this is something that some other users like @webocs requires too and also improves the connector upgrading his driver, maybe we can perform a little effort together to finally merge this driver update. We have human resources to help on all what is needed to get these changes merged, like proposing the changes at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @arsa-dev, apologies for letting this lslide.
I've added my review comments (nothing major). In line with the project's development practices, please also drop the merge commit and use git rebase master
instead.
Other than that the PR should be ready to merge.
Unfortunately I won't be able to attend to the PR again until the next weekend due to conscription. If an earlier release is needed, I'll try to get one of the other maintainers to assist (no guarantee though).
lib/mongodb.js
Outdated
@@ -23,13 +22,14 @@ const ObjectIdValueRegex = /^[0-9a-fA-F]{24}$/; | |||
const ObjectIdTypeRegex = /objectid/i; | |||
|
|||
exports.ObjectID = ObjectID; | |||
exports.ObjectId = ObjectID; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this was a carry-over from #639. However, is there any reason for the rename?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, bson renamed this to match class names. See https://github.com/mongodb/js-bson/blob/main/src/bson.ts#L92-L95. I think that is also a good idea to start using (or at least export) new class name since this changed at new bson versions.
re Juggler: If you can open a PR to fix the issue, I'll help push it through and get it released as well. |
Hi @achrinza, unfortunately did not had time in the week, I'll be working on it today |
45846ac
to
1874329
Compare
Hi @arsa-dev, many apologies for the delay on reverting; I've discussed with the other maintainers on the best way to handle major upgrades to database connectors (which we consider "critical packages"), and how we can convey the right messaging for new releases which are not battle-tested. In this case, we've agreed to merge and release this as a pre-release so that we can publish this without further delay, and continue to refine it over time. I understand it's been some time since the opening of the PR, but if you're still open, could you consider squashing the PR into a single commit? From there, I'll proceed to merge it as-is and cut a pre-release version. We don't expect any breaking changes as we publish future pre-release versions, just that the changes in this PR may not be considered fully field-tested. |
Signed-off-by: Antonio Ramón Sánchez Morales <[email protected]>
01e7fc6
to
39e7715
Compare
Hi @achrinza, I've pushed the squash commit to the branch. |
Thanks for your contributions, it has been merged! 🎉 |
That sounds like great news @achrinza !! Do we know about an estimated release date? I've contacted our client and he is giving us the chance to wait it if is not taking so long. |
Hi @arsa-dev, I'm aiming towards a release within the next 7 days. |
Hi @achrinza, hope that everything is running well. There is any news about the release process? |
Hi @arsa-dev, apologies for the delay on the release. I've went ahead and cut the first v7 pre-release, If there's any problems with the release, please do open a new issue and consider contributing a fix; Since it's currently tagged as a pre-release, we're able to make more rapid changes and code reviews for v7. |
This pull request extends @apocist work that can be checked at #639
Upgrades connector support to the latest MongoDb driver ^4.6.0.
The latest MongoDB driver allows access to MongoDB Altas cloud infrastructure and loadBalanced databases. Connection to this atlas service is impossible with the 3.x series at this time.
As of writing, various servers are successfully using this currently for Mongo Altas Serverless instances.
Known MongoDb Driver 4.x references found in source files and:
https://github.com/mongodb/node-mongodb-native/blob/4.0/docs/CHANGES_4.0.0.md
https://github.com/mongodb/node-mongodb-native/releases/tag/v4.1.0
Implements #638
NOTE: There are 1 test still failing since new bson.ObjectId object instances are now not meeting lodash isEmpty checks, this should be probably fixed at loopback-datasource-juggler repo. Maybe @achrinza or any other member have another idea, I'll be happy to implement any of them.
Checklist
npm test
passes on your machine