feathersjs-ecosystem / feathers-mongoose

Easily create a Mongoose Service for Feathersjs.
MIT License
189 stars 96 forks source link

Named export 'service' should expose constructor function, not init function #37

Closed BigAB closed 8 years ago

BigAB commented 8 years ago

In order to be consistent with the documentation here: https://github.com/feathersjs/feathers-mongoose#classes-es6

The module also exports a Babel transpiled ES6 class as Service that can be directly extended like this:

import Todo from './models/todo';
import { Service } from 'feathers-sequelize';

class MyService extends Service {
 create(data, params) {
   data.user_id = params.user.id;

   return super.create(data, params);
 }
}

The named export service needs to be the constructor function, not the exported default init() function, and also should be called Service captial S by convention and to match the documentation.

Currently: it exports the same function as the default export, and init function and is named service (with a lowercase s)

import * as hooks from './hooks';
import service from './service';

Object.assign(service, { hooks, service });

export default service;

https://github.com/feathersjs/feathers-mongoose/blob/master/src/index.js#L4

daffl commented 8 years ago

With #36 and v3.0.1 require('feathers-mongoose').Service and import { Service } from 'feathers-mongoose' should already work. It is being set in https://github.com/feathersjs/feathers-mongoose/blob/master/src/service.js#L198 which is the default export. Can you double check?

BigAB commented 8 years ago

Well actually, you are right, it is working because of that line. Which is kind of funny though because of this line: https://github.com/feathersjs/feathers-mongoose/blob/master/src/index.js#L4

So you actually want the default import to have both .service(), and .Service() methods? And you have:

import mongooseService from 'feathers-mongoose';
mongooseService === mongooseService.service // true
mongooseService.service === mongooseService.Service // false

I find that a little confusing, but okay.

ekryski commented 8 years ago

Ya that is the intended behaviour.

Come to think of It, it is a bit weird though. Not sure why @daffl initially did it that way. I ended up just following suit with the other adapters.

Maybe simply to just not have the new keyword or perhaps, because when we started this process we weren't sure if we were going to keep backwards compatibility for extending adapters. We ended up doing so, so maybe it was just the quick way it was patched in.

daffl commented 8 years ago

Oh, I did that not to break @BigAB's initial setup (the only user of v3.0 known to me ;) where he had to use .service instead of the default. So to clarify

require('feathers-mongoose') // -> initialization function
require('feathers-mongoose') == require('feathers-mongoose').service // -> backwards compatibility
require('feathers-mongoose').Service // -> constructor function

If that's still confusing we could fix it in a 3.1. I don't think it would break for anybody else.

BigAB commented 8 years ago

I think you should get rid of the named service export (the lowercase one) $0.02