feathersjs-ecosystem / feathers-knex

Service adapters for KnexJS a query builder for PostgreSQL, MySQL, MariaDB, Oracle and SQLite3
MIT License
112 stars 59 forks source link

Expose Knex lib #23

Closed ekryski closed 8 years ago

ekryski commented 8 years ago

So that end users don't need to require it separately we should just expose the underlying knex lib. Just in case people need access to it.

daffl commented 8 years ago

This is available already as service.knex. We should probably mention that in the documentation.

ekryski commented 8 years ago

I would prefer to have it exposed top level. For Knex it doesn't really matter at all. Just working across all the database adapters I'd like to keep their interfaces as similar as possible. Same inputs going in when initializing an adapter, same module definition when requiring. Currently it's beginning to deviate.

In the case of Mongoose, Waterline or Sequelize you may need access to some of their internal objects for testing, creating Schemas, extending a service, etc. So my thought was to expose the main ORM at the top level along side the service adapter so that users don't need to require the same module (or potentially a different version) again.

// so you can do in ES6
import { knex } from 'feathers-knex';

// or using commonjs
var knex = require('feathers-knex').knex;
daffl commented 8 years ago

I don't think this is the case for at least Sequelize and Waterline since you are only providing the initialized models and the adapters don't have to know about the actual libraries at all.

Exposing the Knex object like that makes sense although I'd much rather go the other way and require users to always supply the knex instance (instead of optionally initializing it from the options in https://github.com/feathersjs/feathers-knex/blob/master/src/index.js#L38 which is the only place where we are actually using the Knex library dependency).

That's how we've been doing it with the other libraries, too and I found that offloading library initialization to the user removed a lot of parameter type checking and library initialization boilerplate we have to do on our side. For example, I think that the entire MongoDB adapter connection logic at https://github.com/feathersjs/feathers-mongodb/blob/master/lib/index.js#L31 could just be replaced with expecting an initialized collection in the next version.

ekryski commented 8 years ago

Yes I had been thinking the same thing for mongoose as well. There is a bunch of logic around setting up a connection and because we're re-implementing it, it doesn't support replica-sets, sharding, etc.

We're probably better off to let the user do it and document how to set it up. The down side to this is that it is no longer just a couple lines to setup a feathers service. You'll have to initialize the database connection/ORM first. Meh.

ekryski commented 8 years ago

If you want to go that route, then let's do that.

class Service {
  constructor(knex, options) {
    if(!knex) {
      throw new Error('initialized Knex needs to be provided');
    }

    if(typeof options.name !== 'string') {
      throw new Error('No table name specified.');
    }

    this.name = options.name;
    this.id = options.id || 'id';
    this.paginate = options.paginate || {};
    this.knex = knex;
  }
}

I'll make sure that feathers-mongoose behaves this way, then we'll just need to circle back to feathers-knex. @daffl if you agree then we'll close this issue and create a more appropriate one for this change.

marshallswain commented 8 years ago

I think this makes sense. For mongoose, it means people will be more likely to set up services on the same machine to share the existing mongoose connection, which is what you'd want to do in production, anyway.

ekryski commented 8 years ago

@marshallswain within a single app yes, but the whole goal of feathers from the very beginning has been to make it easy to create great micro-service APIs from day one.

That means each individual app could only have one service endpoint and be living on a totally separate node (either physical or virtual). Of course they could also be on the same machine. If you are spinning up a bunch of small feathers apps then you'll have to duplicate the mongoose (or any other db) connection stuff. Even though there is duplication, this is still fine because you might have one service talk to one mongo cluster, another service talk to a totally separate one or a different database entirely!

I've been playing with Docker and feathers a bit to see how micro services can play with one another. I'll be documenting all of it. We essentially have 3-4 clustering mechanisms, each with their tradeoffs or better fit for certain scenarios. It's pretty sweet though that this is finally starting to come together :smile:

ekryski commented 8 years ago

Alright so we are not going to be exposing the knex lib. The developer needs to take care of that.