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

Saintedlama/mongodb native #256

Merged
merged 47 commits into from
Dec 2, 2015
Merged

Saintedlama/mongodb native #256

merged 47 commits into from
Dec 2, 2015

Conversation

saintedlama
Copy link
Collaborator

@watson @sorribas @mafintosh Found no better place to discuss this branch. It's currently no pull request but an experimental implementation of using mongodb-native driver in the background instead of mongodb-core driver.

The reason for this is that mongodb-native implements a lot of functionality on top of mongodb-core that we would need to reimplement in mongojs too to fix our current 37 open issues. For example:

Distilled: It's auth, topologies and cursor handling that would need to be implemented as the mongodb-native driver does, which means we would need to maintain this code too!

The branch here is quite experimental and has a lot of TODOs in it. Nothing that could not be fixed but some stuff that has to be discussed.

Currently the branch suffers from two "problems":

  1. cursor.explain is broken due to a bug in mongodb-core (https://github.com/christkv/mongodb-core/issues/50)
  2. I'm experiencing problems with EventEmitter, Istanbul, Node.js 4.1.1 and Harmony Proxies. but this only affects coverage runs

…ongojs and access all bson types

All other tests are currently failing
@watson
Copy link
Collaborator

watson commented Oct 13, 2015

@saintedlama Wow, you've put a lot of work into this I can see 🎩 I don't have the mental capacity at the moment to weigh the pro/cons, so I'll get back on that later.

@watson
Copy link
Collaborator

watson commented Nov 13, 2015

@sorribas thanks for explaining 😃
@saintedlama I think your logic for reverting the dependency to mongodb is sound - feel free to merge when you feel it's ready 👍

@saintedlama
Copy link
Collaborator Author

@watson @sorribas @mafintosh The failing test is a consequence of using mongodb-native. I think but I'm not quite sure that mongodb-native has some bug when it comes to explain. Already reported the issue but got a reply that everything works as designed (christkv/mongodb-core#50) - Since explain functionality is not some production critical functionality I'd prefer to adapt the tests to expect the current behavior.

@watson @sorribas I could use a little help with the pull request 😄 - just search for "TODO"

@saintedlama
Copy link
Collaborator Author

Explain bug christkv/mongodb-core#50 was fixed by commit mongodb-js/mongodb-core@4b98902

@watson
Copy link
Collaborator

watson commented Dec 1, 2015

@saintedlama I just installed the branch into one of my projects and ran my test-suite. Completed without any hiccups 😄 🎩

Well a few hiccups, but I think that's my computer doing weird stuff - the tests ran extremely slowly, but seemed to run just as slow when running with mongojs v1.4.1... weird.

Good job with all the tests btw!! Really hard work I could imagine 😄

@mafintosh
Copy link
Collaborator

@saintedlama wow good job

@saintedlama
Copy link
Collaborator Author

I'll integrate the branch today in a bigger project of mine. If that runs as smooth as your test run @watson I'll merge and do a release (major, no beta). @watson @mafintosh ok for you?

@mafintosh
Copy link
Collaborator

@saintedlama sounds good 👍

@watson
Copy link
Collaborator

watson commented Dec 2, 2015

👍

@saintedlama
Copy link
Collaborator Author

Could add a cross platform shelljs based release script to get well done releases - ok for you? ❤️ shelljs since I'm working from a windows machine 😱

saintedlama added a commit that referenced this pull request Dec 2, 2015
Use mongodb-native instead of mongodb-core driver
@saintedlama saintedlama merged commit 2111916 into master Dec 2, 2015
@saintedlama saintedlama deleted the saintedlama/mongodb-native branch December 2, 2015 09:07
@mafintosh
Copy link
Collaborator

@saintedlama seems that 2.0 is working great. just wanted to say great job! if shelljs is easier for you feel free to change it

@saintedlama
Copy link
Collaborator Author

@mafintosh I'm also quite happy with 2.0 🎉 great day 🍻

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.

4 participants