RPC actions can now be a constructed object rather than a literal. #327

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@pygy
Contributor
pygy commented Dec 11, 2012

This is lighter from a memory standpoint, especially for large handlers.

Example:

function RPC(req,res,ss){
  this.req = req;
  this.res = res;
  this.ss = ss;
}

RPC.prototype.sendMessage = function(message) {
  if (message && message.length > 0) {          // Check for blank messages
    this.ss.publish.all('newMessage', message); // Broadcast the message to everyone
    return this.res(true);                      // Confirm it was sent to the originating client
  } else {
    return this.res(false);
  }
}

exports.actions = function(req, res, ss) {
  req.use('session');
  return new RPC(req,res,ss);
};

Something tells me it is actually an oversight :-)

@pygy pygy Actions can now be a constructed object rather than a literal.
This is lighter from a memory standpoint, especially for large handlers.

Example:

```
function RPC(req,res,ss){
  this.req = req;
  this.res = res;
  this.ss = ss;
}

RPC.prototype.sendMessage = function(message) {
  if (message && message.length > 0) {          // Check for blank messages
    this.ss.publish.all('newMessage', message);     // Broadcast the message to everyone
    return this.res(true);                          // Confirm it was sent to the originating client
  } else {
    return this.res(false);
  }
}

exports.actions = function(req, res, ss) {
  req.use('session');
  return new RPC(req,res,ss);
};
```
821690c
@paulbjensen
Contributor

It looks nice, I have a crazy idea.

Would it be possible to use this feature to make an rpc file use multiple RPC functions, providing RPC actions you can plug into an rpc file and re-use across rpc files?

@pygy
Contributor
pygy commented Dec 12, 2012

a mixin? Sure.

var common_responders = require(...);
_.extend(RPC.prototype, common_responders);
@owenb
owenb commented Dec 14, 2012

Interesting idea. It doesn't look as pretty as a closure, nor is it as easy to type, though I can see why it would be faster and more memory efficient.

Without thinking it through too much or testing, from what I can see, this pull request would break existing behaviour?

I don't want to do that for 0.3, but happy to see if we can come up with a better RPC file format for 0.4.

@pygy
Contributor
pygy commented Dec 14, 2012

It wouldn't break any normal looking code, since this in the current version is useless.

I can think of the following breaking behaviors, none of which are likely to occur in practical code:

return {
    foo: function(i){ i = i ? parseInt(i,10) : 0; this.call(this, ++i) }, // A stack? Which stack?
    bar: function(){ if (typeof this !== 'function') throw "Eeewww"),
    baz: function(){ this.__proto__.quux = 6 } // relyng on the fact that 
                                               // Function.prototype is "global".
}

To make it short, code that doesn't rely on the nature of this in the handlers will be fine.

@pygy
Contributor
pygy commented Feb 8, 2013

Still not convinced it should make it for 0.3.3?

With the current code, the method object is thrown away after being called. I don't think anyone would use any of the above patterns in real code...

@owenb
owenb commented Apr 14, 2013

Hey there

Sorry for the delay getting back to you. I have been considering this :)

I've decided to keep everything as it in in 0.3. However, even though 0.3 RPC code must be compatible with rts-rpc, the module which will provide this functionality in 0.4, I will look into ways to making the code more memory efficient. If it's not possible, I'm already planning to release a much improved RPC solution later this year and will definitely bear this in mind then.

Thanks for taking the time to do this, and apologies again for the delay responding.

Owen

@owenb owenb closed this Apr 14, 2013
@pygy
Contributor
pygy commented May 7, 2013

I missed your reply too...

For all practical purposes the patch is backwards compatible, since the old behavior is still available, bar some quirky abuse of the fact that this in the handler is currently the handler itself, which isn't even documented. I would seriously question the sanity of anyone who would rely on this...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment