Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support for native MongoDB driver #24

Merged
merged 5 commits into from
Jan 21, 2018

Conversation

lirbank
Copy link
Contributor

@lirbank lirbank commented Jan 12, 2018

This PRs makes the package work with the native MongoDB Node.js driver, as well as with mongoist.

The tests pass but I’ve not added any new to test with the native driver. Hope that’s OK?

@bradvogel
Copy link
Contributor

can you add a unit test?

@ghmeier
Copy link
Contributor

ghmeier commented Jan 16, 2018

One potential problem I see with this is that it will provide unexpected results if the manofish/mongojs driver is provided since the instance of find there expects a callback.

@gastonelhordoy
Copy link

The callback is actually optional. Check the function return type and and code handling the missing callback.

@lirbank
Copy link
Contributor Author

lirbank commented Jan 16, 2018

@ghmeier is manofish/mongojs a supported driver? I was under the impression only mongoist was supported. Would it be sufficient to update the README to point out the supported drivers are mongoist and, with this PR, native mongodb?

@bradvogel will update with unit tests soon (hopefully tomorrow).

@lirbank
Copy link
Contributor Author

lirbank commented Jan 18, 2018

Ok, I've added tests and updated the README that the native driver is supported. Have a look and let me know if anything else is needed.

WRT the tests, I had some issues with looping the tests through both drivers. I couldn't manage to get the concurrency to operate correctly, so the tests where not reliably running with the correct driver (the driver was replaced in the before section before the tests had completed, stuff like that). Instead of spending more time on it I decided to just run the whole test suite two times, one for each driver, as you can see in package.json. Hope that's ok.

@bradvogel
Copy link
Contributor

Great thanks! Will review in the next 2days. Sorry for the delay

@bradvogel bradvogel merged commit 434e910 into mixmaxhq:master Jan 21, 2018
@bradvogel
Copy link
Contributor

Thanks! Released as 6.1.0.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants