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

RPC Actions can now be an object created with new rather than a literal. #407

Merged
merged 1 commit into from
Oct 23, 2013

Conversation

pygy
Copy link
Contributor

@pygy pygy commented Oct 17, 2013

This is a revival of #327, now that Paul is at the helm I hope it can be revisited.

This tiny patch allows to use objects created with new as RPC handlers, rather than object literals with closures. It removes pressure from the allocator/garbage collector, since otherwise, closures have to be created for each requests, most of which are not used anyway.

The resulting code is a bit uglier but much more efficient, especially for large RPC APIs.

Example (revisiting the "new_project" sample):

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);
};

The old method can still be used for all practical use cases.

The patch may theoretically break code that relies on the fact that, without the patch, this in a handler is the handler itself. I can't see anyone sane rely on that.

@ghost ghost assigned paulbjensen Oct 18, 2013
@RomanMinkin
Copy link
Contributor

Cool idea! 👍

@paulbjensen
Copy link
Contributor

Hi, thanks for submitting this. I'll take a look at it later today.

@RomanMinkin
Copy link
Contributor

My concern is:

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

It's kinda annoying to put it in each file within server/rpc folder. Maybe it's better to put it outside RPC files. For example put it under ss.api?

@pygy
Copy link
Contributor Author

pygy commented Oct 19, 2013

You need one per API, since you attach the methods to the prototype.

// store this wherever relevant... 

function factory (actions) { // I'm sure there's a better name :-)
  var RPC = function (req,res,ss){
    this.req = req;
    this.res = res;
    this.ss = ss;
  }
  RPC.prototype = actions
  return RPC
}

Then, in server/rpc/demo.js

RPC = factory({
  sendMessage: function(message) {
    // ...
  }
})

paulbjensen added a commit that referenced this pull request Oct 23, 2013
RPC Actions can now be an object created with `new` rather than a literal.
@paulbjensen paulbjensen merged commit 2346634 into socketstream:master Oct 23, 2013
@paulbjensen
Copy link
Contributor

Thanks. At some point we'll want to document this in the guide, and give a description of the pros/cons

@kulicuu
Copy link

kulicuu commented Oct 24, 2013

This sounds very interesting. Code snippet of example usage possibility?

On Thu, Oct 24, 2013 at 1:54 AM, Paul Jensen notifications@github.comwrote:

Thanks. At some point we'll want to document this in the guide, and give a
description of the pros/cons


Reply to this email directly or view it on GitHubhttps://github.com//pull/407#issuecomment-26953344
.

@pygy
Copy link
Contributor Author

pygy commented Oct 24, 2013

Code snippet of example usage possibility?

It's all in the first post. Contrast with server/rpc/hello.js.

@pygy pygy deleted the RPCobject branch October 24, 2013 14:32
@pygy
Copy link
Contributor Author

pygy commented Oct 25, 2013

BTW, thanks for accepting this :-)

@paulbjensen
Copy link
Contributor

You're welcome, thanks for contributing it, there's always room for
improvements like this.

On 25 October 2013 16:41, pygy notifications@github.com wrote:

BTW, thanks for accepting this :-)


Reply to this email directly or view it on GitHubhttps://github.com//pull/407#issuecomment-27102684
.

Paul Jensen
07914 171 345

@RomanMinkin
Copy link
Contributor

Hi guys,

I still think that we need to add code from #407 (comment) to SocketStream src. Not sure where exactly, probably should be in ss.api, maybe inside lib/socketstream.js:

api.add('rcpProvider', function(){
  ...
  });
};

Still kind of ugly, but i think since SS is a framework it should not be up to user, too complicated =)

...or maye should be something like following inside app.js:

var ss = require('socketstream');

ss.use('rcpProvider');

Thoughts?

@pygy
Copy link
Contributor Author

pygy commented Oct 25, 2013

I don't know where to put this either, since files in API are not supposed to require SocketStream. The ss.api object is injected in the exports.actions callback, but the provider must be called outside of it if you want it to be useful.

Maybe you could put it as a small helper file, either in app/server or in app/server/rpc, using the filter option of apitree to exclude it from the API.

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

Successfully merging this pull request may close these issues.

4 participants