Skip to content
This repository has been archived by the owner on Apr 20, 2018. It is now read-only.

feathers-hooks is changing the scope of makeWrapper() #17

Closed
marshallswain opened this issue Feb 4, 2015 · 12 comments · Fixed by #18
Closed

feathers-hooks is changing the scope of makeWrapper() #17

marshallswain opened this issue Feb 4, 2015 · 12 comments · Fixed by #18

Comments

@marshallswain
Copy link
Member

When using feathers-hooks, the scope of the makeWrapper() function is the global node object. Since that same scope is passed to the feathers-mongodb service, it causes an error here of TypeError: Cannot call method 'find' of undefined. (this.collection is undefined on the global node object).

If I comment out the users.before() line in the example code below, the scope of this inside makeWrapper() is the service with its methods.

I've tested both the latest commit of feathers-hooks and the 0.4.0 version from npm.

So far this has only been a problem when using feathers-mongodb because of its use of this inside the methods.

Here is the simple server.js I'm using to test:

var feathers = require('feathers'),
  fHooks = require('feathers-hooks'),
  mongo = require('mongoskin'),
  fMongo = require('feathers-mongodb'),
    bodyParser = require('body-parser');

var app = feathers()
    .use(feathers.static(__dirname + '/public'))
    .configure(feathers.rest())
    .use(bodyParser())
    .configure(fHooks())
    .configure(feathers.socketio())
    .configure(feathers.errors());

// Synchronous connect to db
var db = mongo.db('mongodb://localhost:27017/feathers-tuts');
var userService = fMongo({collection:db.collection('users')});

// The users endpoint must match the one setup on config.store.
app.use('api/users', userService);
var users = app.service('api/users');
// Dummy hook
function hookMe(hook, next){
    next();
}
users.before({find:[hookMe]});

// Start the server.
var port = 8080;
app.listen(port, function() {
  console.log('Feathers server listening on port ' + port);
});
@daffl
Copy link
Member

daffl commented Feb 4, 2015

Is that also happening with latest master? Feathers makeWrapper only returns a function which should be dynamically bound, no?

@marshallswain
Copy link
Member Author

Feathers makeWrapper only returns a function which should be dynamically bound, no?

Correct. I thought that the problem might be here in feathers: https://github.com/feathersjs/feathers/blob/master/lib/mixins/promise.js#L24, because it's not using call or apply to change context? But it's working without feathers-hooks, and I don't see why it would be any different with it or without it, unless it's changing somewhere earlier in the stack.

It's happening with the latest master("feathers-hooks": "git://github.com/feathersjs/feathers-hooks.git",) and with ^0.4.0. Below is an error with stack trace. I've shortened the folder paths. The ... is the root folder of the project.

TypeError: Cannot call method 'find' of undefined
    at Proto.extend.find (.../node_modules/feathers-mongodb/lib/mongodb.js:67:21)
    at .../node_modules/feathers/lib/mixins/promise.js:8:30
    at self.(anonymous function) (.../node_modules/feathers-mongodb/node_modules/uberproto/lib/proto.js:61:21)
    at fn (.../node_modules/feathers-hooks/lib/before.js:24:21)
    at hookCallback (.../node_modules/feathers-hooks/lib/commons.js:25:12)
    at Object.hookMe (.../server.js:28:2)
    at Object.<anonymous> (.../node_modules/feathers-hooks/lib/commons.js:28:24)
    at Object.<anonymous> (.../node_modules/feathers-hooks/lib/before.js:31:15)
    at Object.self.(anonymous function) (.../node_modules/feathers-mongodb/node_modules/uberproto/lib/proto.js:61:21)
    at Object.<anonymous> (.../node_modules/feathers-hooks/lib/after.js:18:21)

@daffl
Copy link
Member

daffl commented Feb 4, 2015

Can you try

var userService = fMongo({collection: 'users'});

Looking at the error at https://github.com/feathersjs/feathers-mongodb/blob/master/lib/mongodb.js#L66 it seems as if .collection is initialized wrong (I am pretty sure it currently expects a string).

@marshallswain
Copy link
Member Author

It looks like the problem is happening here: https://github.com/feathersjs/feathers-hooks/blob/master/lib/before.js#L24

this is undefined, which makes apply use the global scope.

@marshallswain
Copy link
Member Author

Oh. I forgot I'm using this branch of feathers-mongodb that I was working on a couple weeks ago:
https://github.com/marshallswain/feathers-mongodb/tree/init-with-db

I don't think there is anything that should be changing the outcome, but let me test with the current master.

@daffl
Copy link
Member

daffl commented Feb 4, 2015

I pointed that out because it is complaining about collection.find not service.find. If you are suggesting that the methods are loosing context it should be more easily reproducable by doing something like:

var feathers = require('feathers');
var fHooks = require('feathers-hooks');

var app = feathers()
    .use(feathers.static(__dirname + '/public'))
    .configure(feathers.rest());

// The users endpoint must match the one setup on config.store.
app.use('api/users', {
  number: 10,
  find: function(params, callback) {
    callback(null, [{
      id: this.number;
    }])
  }
});
var users = app.service('api/users');
// Dummy hook
function hookMe(hook, next){
    next();
}
users.before({find:[hookMe]});

// Start the server.
var port = 8080;
app.listen(port, function() {
  console.log('Feathers server listening on port ' + port);
});

@marshallswain
Copy link
Member Author

Yeah, good point on doing the simplest test. Even that example fails with the same problem. console.log(this) from within find, using the example you just posted, produces the global scope:

{ ArrayBuffer: [Function: ArrayBuffer],
  Int8Array: { [Function: Int8Array] BYTES_PER_ELEMENT: 1 },
  Uint8Array: { [Function: Uint8Array] BYTES_PER_ELEMENT: 1 },
  Uint8ClampedArray: { [Function: Uint8ClampedArray] BYTES_PER_ELEMENT: 1 },
  Int16Array: { [Function: Int16Array] BYTES_PER_ELEMENT: 2 },
  Uint16Array: { [Function: Uint16Array] BYTES_PER_ELEMENT: 2 },
  Int32Array: { [Function: Int32Array] BYTES_PER_ELEMENT: 4 },
  Uint32Array: { [Function: Uint32Array] BYTES_PER_ELEMENT: 4 },
  Float32Array: { [Function: Float32Array] BYTES_PER_ELEMENT: 4 },
  Float64Array: { [Function: Float64Array] BYTES_PER_ELEMENT: 8 },
  DataView: [Function: DataView],
  DTRACE_NET_SERVER_CONNECTION: [Function],
  DTRACE_NET_STREAM_END: [Function],
  DTRACE_NET_SOCKET_READ: [Function],
  DTRACE_NET_SOCKET_WRITE: [Function],
  DTRACE_HTTP_SERVER_REQUEST: [Function],
  DTRACE_HTTP_SERVER_RESPONSE: [Function],
  DTRACE_HTTP_CLIENT_REQUEST: [Function],
  DTRACE_HTTP_CLIENT_RESPONSE: [Function],
  global: [Circular],
  process: {}
  GLOBAL: [Circular],
  root: [Circular],
  Buffer: {},
  setTimeout: [Function],
  setInterval: [Function],
  clearTimeout: [Function],
  clearInterval: [Function],
  setImmediate: [Function],
  clearImmediate: [Function],
  console: [Getter],
  _super: [Function] }

@marshallswain
Copy link
Member Author

It looks like the fix was as simple as putting a var self = this; in getMixin() for before.js and using that inside fn() here: https://github.com/feathersjs/feathers-hooks/blob/master/lib/before.js#L24

@marshallswain
Copy link
Member Author

after() hooks seem to work fine.

@daffl
Copy link
Member

daffl commented Feb 4, 2015

You're right, good catch! Found the issue, will land a patch and make a new release tonight.

@marshallswain
Copy link
Member Author

Thanks. And thanks for your help!

@daffl
Copy link
Member

daffl commented Feb 5, 2015

This issue as well as the others are fixed and version 0.5.0 is released. /cc @ekryski

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants