custom contentWriters (again!) about node-restify HOT 19 CLOSED

My fork over at https://github.com/benhowes/node-restify contains this change and a couple of other small changes later on that will allow the content writer to make changes to the response, which in some cases is desirable, such as when you're using JSONP and you want to always send a 200 so that the a callback is always fired.

I have additionally published the JSONP contentWriter that I wrote to go with it, which seems to be working quite well with the changes on my fork, which is at https://github.com/benhowes/restify-jsonp

As far as I can tell, the changes that I have made are not going to have any affect on existing applications, which was something that I kept in mind when making changes :)

Ben

from node-restify.

Hey Ben,

Sorry, I was doing a little work travel yesterday. A couple comments:

• I'd actually be fine just merging application/jsonp into restify core (not in coffee though ;) ). I doubt you're the only person who will want it. Not sure if you want to go that route or not?
• I looked at the changes you made; I marked up one. but I don't really understand what the change you made to go from

_opts.<foo> to this.options.<foo>

is doing?

Anyway - let me know; we'll get this worked in one way or another :) and thanks so much for the contributions/help!

m

from node-restify.

That's no worries!

• That sounds awesome, so long as I get some proper testing sorted out for it then there is no reason not to I guess.
• The type of changes that you mentioned are so that the contentWriter can have an effect on what happens there, because the contentWriter.call(this, ..) doesn't provide access to the _opts variable, but there is already a copy on the this so I just switched to use them. For my application I only really care about the _code so I can always send a 200, but I changed them all for consistency in case some other contentWriter I am not considering would want to use them.

That's no worries, this project is still making my life a lot easier at the moment so it's the least I can do!

Ben

from node-restify.

1. Ok cool - so, I'll just wait for a pull request from you for for merging it all in?
2. Got it. I missed what you were trying to do. Should probably rationalize what to call what's exposed since apps will be dependent on it (i.e., I don't really want to surface a use _code type paradigm).

And awesome that it's working out well for you!

m

from node-restify.

1. The thing with the jsonp handler (and potentially other handlers) is that it needs to get params from the request query string. I have done this so far by injecting a function into the pre queue for all routes, which is pretty messy! I don't particularly want to pull request on that, just because it doesn't make for good code and would require developers to remember to put it in the pre chain all the time.

In terms of making a good API I don't think that just copying query string parameters across would solve all the potential problems. I think that the most sustainable way is to just have a link in the pre chain, but perhaps make it global? So have an additional option when creating the server that allows a set of global pre chain links, that will be called before anything else. I reckon this would make a nice way to put all the standard stuff for all the routes in one place. I would then add a line or 2 in http.Server.prototype._mount which would prepend the pre chain with the server._config.handlers.pre. All contentWriters could then have an optional exports.handler.pre to do something at the beginning of the pre chain.

I appreciate this is quite a few changes, but I think it would be awesome to build something that is easily extendible going forwards. Let me know if that sounds okay, or if you have any other ideas :)

2. I reckon headers, code, time are the useful ones, so perhaps loose the underscore from those ones?

Ben

from node-restify.

Hey Ben,

1. Yeah, I thought about this when I put that stuff in initially, and for whatever (silly) reason, I didn't append them in. Probably the best thing to do is to just make the contentWriters and contentHandlers api to take the req/res pair. It could probably be done without breaking the current api; i.e., make the callback something like:

  contentHandlers: {
    'application/foo': function(body, req, res) {
      return JSON.parse(body);
    }
  },
  contentWriters: {
    'application/foo': function(obj, req, res) {
      return JSON.stringify(obj);
    }
  },

But that said, I'm ok with breaking that part of the api and just bumping up the version (I still haven't even announced this project yet...) to 0.4.

That doesn't seem mutually exclusive from defining a global pre/post chain, so I'm coll with adding that too.

2. Agreed. I'm not sure there's anything left :)

Thanks again - and if you want me to take on any of this, I certainly can.

m

from node-restify.

Hey Mark,

I've done most of this, other than the global pre chain. Couple of problems I've run into:

• I fail 5 of the tests, though not really sure what's causing the problem - have these all been running okay before? I have pasted my results here: https://gist.github.com/1101369
• I thought my plans for separating out the contentHandlers and writers was coming together really well until I saw the formidable stuff in _parseRequest. I think that I can hoist it out and make it a content handler too, though I think there would have to be a blocking mechanism to prevent returning without finishing processing the form and also no processing of the parts would happen until the form was completely uploaded. I'm sure this is going to have a performance hit, though I have never used formidable and don't really know if this would be a big problem? If its not then I will hoist and align with the rest of the code that I have done.

Ben

from node-restify.

Hi Ben,

Yeah they all pass off the 'official' master branch. The tests can be a little hard to debug since what it's doing is starting a server on a unix domain socket, and all traffic goes over that. The easiest way to get some more info about what's blowing up is to turn on trace logging (just do a log.setLevel('Trace') up by the require() block), and run just that client test (I looked superficially at your current fork, and didn't see anything obvious where you're referencing 'body'). Not sure if that fork is up to date with your local changes though. If it's not obvious what's broken after that, put all your code up on your fork, and I'll pull it down and take a look (besides, that'll be easy to diff against master).

I forgot about formidable (I swear, multipart forms are such a pain in the ass; if it wasn't for curl -F I wouldn't even have it in there). So formidable works well, but it's totally a one-off since you basically hand it a socket, it sends the client an HTTP 100, and then it handles the upload and parsing. I'm not really sure I follow what you're thinking how it would come out (i.e., if 'blocking' means literally stop anything else from happening, yeah, we can't have that). Can you gist me something of what the stuff that's !formidable looks like?

m

from node-restify.

Hey Mark,

I had made a school boy error in my haste! None of the tests that were needing to get a body out of an application/json content type were working. All sorted now, which feels nice :)

So with the formidable stuff, I hadn't really looked at how it works... The changes that I have made should have no effect on this and I guess we will just have to leave it as is, seen as some pretty big changes would be needed.

So, on my fork jsonp is working for me! I think a test bench could be done, but it would have to use the eval() function to see if the response is well formed JS. I might get a chance later this week to do it, unless you fancy it? ;)

Ben

from node-restify.

Hey Ben,

Awesome - glad it's working. And I can wait for when you get the tests ;).

m

from node-restify.

Okay man, I'll ping you a pull request when I've done that, probably at the weekend.

Ben

from node-restify.

Hey Ben,

Any update on this? If you have it in a state where it's passing the existing tests and you don't have time to add tests for JSONP, that's cool - I'll just mark it as "experimental". I'm going to make some improvements in the next couple days (and bump to an 0.4) and get this working on node 0.5, and I didn't want to start touching that code without your drop.

Thanks!
~Mark

from node-restify.

Hey,

Sorry for the silence! Yes, it is working for me. I have put one test in the get file, but I never got round to doing more. I do intend to, I have just gotten to a bit of crazy busy stage with my project.

I'm pretty sure that my fork is all up to date :)

Ben

from node-restify.

By your fork is up to date you mean I should just pull whatever you have off master?

from node-restify.

Yeah, my forks master branch contains all my working changes at the moment.

Sorry I've been rubbish at keeping up with this lately. I do intend to commit more time to this after I have gotten v1 of my project out and working :)

from node-restify.

Hey Ben-

I pulled this into master, I'll publish some time next week as part of 0.4. Have a few other things to push in.

from node-restify.

Closing this out - I published 0.4.0. If you get a chance to add some more tests, I'll gladly take a pull request.

from node-restify.

Cool beans, I'm a couple of weeks away from some form of launch now. I am thinking about having a look at xml as well as much as I personally would prefer json!

Thanks,
Ben
------Original Message------
From: mcavage
To: [email protected]
Subject: Re: [node-restify] custom contentWriters (again!) (#35)
Sent: 12 Aug 2011 21:47

Closing this out - I published 0.4.0. If you get a chance to add some more tests, I'll gladly take a pull request.

Reply to this email directly or view it on GitHub:
#35 (comment)

Ben

from node-restify.

Yeah, XML was on my original list, but I just sort of let it languish since
I personally didn't have a need :\

On Fri, Aug 12, 2011 at 3:40 PM, benhowes <
[email protected]>wrote:

Cool beans, I'm a couple of weeks away from some form of launch now. I am
thinking about having a look at xml as well as much as I personally would
prefer json!

Thanks,
Ben
------Original Message------
From: mcavage
To: [email protected]
Subject: Re: [node-restify] custom contentWriters (again!) (#35)
Sent: 12 Aug 2011 21:47

Closing this out - I published 0.4.0. If you get a chance to add some more
tests, I'll gladly take a pull request.

Reply to this email directly or view it on GitHub:
#35 (comment)

Ben

Reply to this email directly or view it on GitHub:
#35 (comment)

from node-restify.