This seems to be the more appropriate fix for the .log error

[cha0s]

I previously submitted a PR for this issue. I get "Cannot call method 'emit' of undefined" and I'm not sure why this ever works, as you can see at https://github.com/pocesar/promised-jugglingdb/blob/master/lib/schema.js#L479 the log function is trying to do this.emit, but the log = (this|schema).log is not preserving 'this'. This seems to be the correct fix.

[pocesar]

how are you calling the log function? because this in the browser might be window or globals in node, [as it can be undefined (in the case)] and if there's something called log in there, it might backfire

[cha0s]

I am not actually calling it programmatically, jugglingdb itself is calling the function and erroring out. The error seems to occur because the log function is called with global this and if this.emit doesn't exist, then it throws an error. I added a comment that could hopefully help clarify. I could also revert my local to get a more detailed backtrace.

It appears the Travis test failures have to do with an ENOENT error with npm, and not actually due to this change. At least, looking at https://travis-ci.org/pocesar/promised-jugglingdb/jobs/20652668 leads me to believe so. I'm not super familiar with Travis so I could be wrong.

Also I should note, I am using browserify to generate a module that I can include in the browser (jugglingdb on server and client is sexy), and I am including that version on the server as well. That may be what is actually causing this error to manifest. If there is a good reason for calling log with global this then it makes sense to keep doing things this way. However, it seems like a bug.

[cha0s]

Also, https://github.com/cha0s/promised-jugglingdb/blob/master/lib/schema.js#L108 seems to take the same approach as I am here: in fact schema.adapter.log is the exact same function, but this is properly preserved in this case.

[pocesar]

ok, we need to find out where exactly there's a call to adapter.log (or even from inside other adapters, maybe memory?) because it should either: a) be called with log.call(context... b) curried to the expected context of the adapter in this case. I think the problem is calling log as a standalone function, instead of var log = this.log; which makes it lose it's context. (Javascript quirks). maybe it should be written as:

    schema.adapter.logger = function (query){
      var self = this;
      var t1 = Date.now();
      return function (q){
        self.log(q || query, t1);
      };
    };

[cha0s]

We can do that, sure! The funny thing is as I mentioned, schema.adapter.log ( https://github.com/cha0s/promised-jugglingdb/blob/master/lib/schema.js#L108 ) is actually just a wrapper for schema.log, so it's functionally equivalent either way! All I know is that losing the this context is puking whenever "global".emit doesn't exist.

[pocesar]

I see, could you try to do a debug break (using debugger and checking chrome devtools) to see the stack? this should never be the global namespace, it means it's leaking the context somewhere. also I couldn't find anywhere (at least in mysql adapter and the bundled ones) that the log is used. since you are using it in the browser, it's the memory adapter, correct?

[cha0s]

The error occurs on the server, I am using the redis adapter. I'll revert and investigate this problem tomorrow.

Just for kicks, I am using a custom adapter on the client: https://github.com/cha0s/shrub/blob/master/client/modules/jugglingdb-rest.coffee though I can't remember if it is actually enabled since I have been tearing a lot of stuff in this (unstable for now) project! :smiley:

[pocesar]

the redis adapter is doing exactly what I thought, it's losing it's context because it's making a standalone version of log:

https://github.com/jugglingdb/redis-adapter/blob/master/lib/redis.js#L73

so, the resulting function should be either bound to the this context, or "saving" the context using the way I showed (but I'm not sure if it's working), since when this._adapter.logger() is called, the context of the function SHOULD be the adapter and not the Schema class I think

[cha0s]

I see your point but I consider this a bad API design of jugglingdb, because it violates the principle of least surprise to require the consumer of schema.adapter.logger to ensure this is correctly set. If we just follow the pattern of schema.adapter.log and qualify (this PR), then the API works as expected. This may be a personal opinion, and I have no problem maintaining a simple fork! Perhaps we should ask the maintainer of jugglingdb what he thinks? Hey @1602 are you there?

[1602]

@cha0s promises are not needed (because it adds overhead for nothing just syntax), yes API design sometimes bad, we are here to fix it.

[cha0s]

@1602 I respect that we have different opinions on the usefulness of promises, but the issue we are discussing is present in your tree as well. Could you weigh in on the discussion we've had so far and give us your opinion as maintainer of that API?

[1602]

@cha0s I agree with that change.

[pocesar]

I guess this issue can be closed @cha0s ? the lastest merge I've made sure to save the context of the logger, so it won't leak to the global scope

[cha0s]

Yup, the fix looks correct. Thanks! :)