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

Expose Knex lib #23

Closed
ekryski opened this issue Dec 28, 2015 · 8 comments
Closed

Expose Knex lib #23

ekryski opened this issue Dec 28, 2015 · 8 comments

Comments

@ekryski
Copy link
Member

ekryski commented Dec 28, 2015

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
Copy link
Member

daffl commented Dec 29, 2015

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

@ekryski
Copy link
Member Author

ekryski commented Dec 29, 2015

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
Copy link
Member

daffl commented Dec 29, 2015

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
Copy link
Member Author

ekryski commented Dec 29, 2015

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
Copy link
Member Author

ekryski commented Dec 29, 2015

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

  • We won't worry about exposing the underlying ORM lib.
  • We'll make the interface such that a user needs to pass in an initialized connection/ORM object:
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
Copy link
Member

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
Copy link
Member Author

ekryski commented Dec 30, 2015

@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 😄

@ekryski
Copy link
Member Author

ekryski commented Jan 4, 2016

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

@ekryski ekryski closed this as completed Jan 4, 2016
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

No branches or pull requests

3 participants