Allow services to share a common db connection.

[marshallswain]

This is my proposal and PR to optionally enable services to share the same connection. The existing API is unchanged.

[marshallswain]

I'm not sure if it's supposed to be Proto.create() or Proto._create() in the failed commit. The original repo just has create(), but I can't get it to work unless I use _create().

[daffl]

That looks great but the build failed (JSHint: == instead of ===). The _create thing is probably because of feathersjs/feathers#100 although that shouldn't be a problem here since this NPM package should have its own Uberproto instance. I will give it a try later.

[marshallswain]

Looks like it wants that _create(). I've put it in place and the build passes.

[daffl]

Shoot, that shouldn't happen. I'll give it a try and see how this can get fixed.

[marshallswain]

http://thenodeway.io/posts/how-require-actually-works/

It looks like Node's module cache (the module._load() step in that article) is reusing Feathers' instance of Proto.

[daffl]

Ah that's good to know. Maybe we can make it

var create = Proto.create || Proto._create;

And mayb also allow to pass a collection (I guess in which case we wouldn't need the db instance at all)?

this.collection = typeof options.collection === 'string' ? this.store.collection(options.collection) : options.collection;

[marshallswain]

Maybe we can just change the top of Feathers' application.js to work with its own copy of Proto by require'ing lodash first and using _.clone() thus:

var _ = require('lodash');
var Proto = _.clone(require('uberproto'));
// We do not want to support Uberproto's create functionality
// Since our service methods have a method with the same name
Proto._create = Proto.create;
delete Proto.create;

That way we don't have to fix every service that uses Proto.

It looks like we have a straggling copy of feathers in the node_modules directory under feathers-mongodb. I made the above fix to my main copy of feathers and it didn't change anything. It looks like this module is requiring feathers just to work with the errors. It probably needs to import feathers-errors directly.

[marshallswain]

Yeah, we might as well set it up to only require a connected collection. It's simpler, cleaner:

var feathers = require('feathers')
  , mongo = require('mongoskin')
  , mongoService = require('feathers-mongodb')
  , app = feathers();

// First, make the connection.
var db = mongo.db('mongodb://localhost:27017/my-project');

// Use the same db connection in both of these services.
app.use('/api/users', mongoService({collection:db.collection('users')}));
app.use('/api/todos', mongoService({collection:db.collection('todos')}));

app.listen(8080);

[marshallswain]

That will fail until we fix feathers and remove the straggling dev dependency.

[marshallswain]

@daffl, @ekryski, It looks like the build errors with feathers got ironed sometime in the last couple months. Does this look like something I can squash and merge?

[daffl]

Shouldn't be necessary with a gobal .gitignore

[Glavin001]

I disagree. I would have the node_modules and bower_components in the project's .gitignore.

I see global ignore as user or platform specific - Mac has different gitignores (.DS_Store, etc), your person editor (Xcode, Vim, etc) and those go into global gitignore.

The local gitignore for repositories are for the project itself, and platform agnostic -- because we are using Node.js therefore we ignore node_modules. I don't think this should be in your global gitignore. You said it yourself: "There are files that should not be included in Git repositories (like IDE settings, OS generated files, temporary files etc.)." I do not see node_modules as being under this category.
While it may end up being that all of your projects have node_modules in the gitignore I see this as basic scaffolding in the same way we create a package.json file for Node.js projects - a Node.js project has package.json and add node_modules to gitignore, then continue. A new contributor should be able to simply download the repository and get going without having to change their global gitignore -- the project itself should know internally what needs to be ignored. In the same way we may have different coding standards in .editorconfig or .jshintrc files, these are project specific.
Regardless that these will almost always be the same, I see this as a very project specific thing and should be isolated in that way. Also note that some projects actually do include the node_modules directory. While this is not how I do it, to reiterate it is a project specific preference.

Just my quick thoughts.

[daffl]

Fair enough. I always considered it unnecessary boilerplate but with a bigger contributor base telling everyone to use a global .gitignore is pretty tedious. But then we should have the same .gitignore in all projects and I'd also argue to include IDE settings (I keep publishing my .idea folder to NPM because it doesn't honour the global .gitignore or have to add an unnecessary separate .npmignore).

[ekryski]

Ya I'm with @Glavin001 on this and I do think that we should be put IDE dot files in our .gitignore. Also gets hairy as more contributors come to the mix.

[daffl]

👍 Can someone take care of that? We can also remove the .npmignore that ignores the same things. My recommendation is the one I already use globally:

# OS files
.DS_Store
.DS_Store?
.Spotlight-V100
.Trashes
Icon?
ehthumbs.db
Thumbs.db

# IDEs
.idea
.workspace
.metadata

# Node
node_modules
npm-debug.log

# Package managers
bower_components

Suggestions welcome.

[ekryski]

@daffl I think that is good. At least for now. If no one has taken it right away here.

[daffl]

Looks good to me since it is still API compatible. We can merge once you take the .gitignore out (node_modules should be ignored globally, I wrote up a Gist here) and squash the commits.

[marshallswain]

Ok. I believe this should be ready, now.

[daffl]

👍 Awesome! Go ahead and merge it.

[daffl]

I added you as an owner to the package so you can npm publish the tag you just made.

For future releases you can now just use grunt release:patch/minor/major which will bump the version, add the tag, push to GitHub and publish to NPM all in one.

[ekryski]

Rather than doubling up the collection parameter we should have just supported the connection parameter like this in the feathers-mongoose lib. That's really what we are passing, a connection object, not a collection.

[marshallswain]

Seems reasonable to me. So what would the new syntax be?

[ekryski]

I'm gonna do up a PR and then you guys can critique. It will probably be something like this:

var url = 'mongodb://localhost:27017/test';

MongoClient.connect(url, function(err, db) {
   app.use('/api/users', mongoService({
       collection: 'users',
       connection: db
   }));
});

You can choose how you are making your connection to mongo but basically you are just passing the database connection to the mongodb service along with the collection.