restify support

[DonutEspresso]

Hi, we (restify maintainers) are looking at deprecating the existing serve static plugin that ships with restify in favor of something that's being actively maintained. This is one of the modules on the short list, and I wanted to start a conversation around what would be needed to fully support restify. I'd be happy to do the initial work, but thought that starting here first would be most practical.

restify uses the same middleware chaining as express, so ecstatic works out of the box for all the basic happy paths. There are two things I think we'd need to get 100% support:

1. option to always invoke next(), regardless of success/failure. restify uses this for various things, but it's important that next() is always called when ecstatic is done.
2. an option to avoid writing errors (fallback or not) out to the response, and instead pass any error objects on to next(). this can be done as a subset option of the first one, or as a completely independent option (but that seems to make less sense, I think, as I can't imagine enabling option 2 independently).

In short, happy paths can write out to the res as is, but anytime an error occurs, we'd like that error object propagated to next. restify uses events to give consumers fine grained control over different error types, so it's important that we can get at the underlying error object. The cherry on top would be attaching an http status code to that error object (whatever status ecstatic thinks is is most appropriate).

I did some poking around and I think it's doable, but it is going to require a little futzing of the status handlers as well as the recursive cases to ensure that next() is always called. And in some cases, 'error' cases don't always have an error object associated with it, and we'd have to create one (for example, in the scenario that the request is terminated from the client). Would love to get your thoughts. Thanks!

FYI @micahr @yunong.

[micahr]

Seems like a great idea to me.

[dotnetCarpenter]

@DonutEspresso I think your first point can be accomplish by using ecstatic({ handleError: false }).
Play around with that and see if that fits the criteria set out in your second point. If not, then we can look into it.

[DonutEspresso]

@dotnetCarpenter handleError: false gets us partially there, but the error object isn't being passed on. I see res.statusCode being set, so we could infer an error from a non-200 statusCode but there's certainly some loss of fidelity. There are also a couple of other exit points where handleError is not being used, thus not invoking next:

unknown fs errors:
https://github.com/jfhbrook/node-ecstatic/blob/master/lib/ecstatic.js#L124

autoIndex errors:
https://github.com/jfhbrook/node-ecstatic/blob/master/lib/ecstatic.js#L141

client closed request:
https://github.com/jfhbrook/node-ecstatic/blob/master/lib/ecstatic.js#L215

On the other hand, showdir.js looks good, and is using the handleError flag in all error paths AFAICT.

[DonutEspresso]

Also, just to clarify - the success scenarios also need to invoke next(), and that would have to be done via a new option, something like alwaysNext: true. That's not covered by any of the existing flags.

[jfhbrook]

Interesting.

I have to read this thread in more detail and now's a bad time because 2:30am on a worknight. In general, though, I'm down to support restify. I'm even down to break the existing API if necessary. That said, I don't personally have the bandwidth to implement.

[DonutEspresso]

If you don't mind, I can take an initial stab at a pull request in the coming weeks. I see two parts to this:

1. Add a new option, something like alwaysNext: true, which will always invoke next() regardless of success/failure.
2. Ensure handleError: false covers all error scenarios, and invokes next(err). This would be a breaking change, and to be honest I am also not sure what express will do when you call next(err). Alternatively, I could add another option, something like resOnError: false which would skip writing the response in error scenarios.

Effectively, this would turn ecstatic into a callback-like "utility", where the callback is always invoked but responses are only written in success scenarios. In error scenarios, callback is invoked with err.

Thoughts?

[jfhbrook]

Sorry, my personal life has been fucking nuts the last week.

alwaysNext sounds fine. I don't see how it would be applicable outside restify, but I don't see the harm. EDIT: No, I can see the benefit.

Consistent error handling is generally a good idea. It looks like express can handle errors passed to next http://expressjs.com/en/guide/error-handling.html and making "handleError: false" pass errors to the callback is probably something non-middleware-users would like, especially if we don't mutate the request or response but includes the proposed code on err.code or similar. Does restify have a convention for error metadata?

How do express users feel about this?

at any rate, it's probably a good idea to make the default behavior handle errors as it does now.

[jfhbrook]

Another thought: If you need multiple flags to make this work, maybe a "restify" flag that encompasses all the necessary behaviors (as sugar) could be useful. Maybe do similarly for "express" as well just to be fair.

[jfhbrook]

@DonutEspresso btw if you wanna talk about this in irc I'm in freenode, you can find me in #node.js or you can tell me where to find you, either way

[DonutEspresso]

Hey, sorry, been swamped with stuff recently and this got pushed on to my backlog. I have a WIP branch I'll share in the coming weeks when I have some time. Thanks!

[jfhbrook]

Cool @DonutEspresso, don't hesitate to hit me up if you have questions or want to discuss in the meantime. IRC is best.

[jeevank]

@DonutEspresso Any updates on this? It'd be great to get Restify support.

[DonutEspresso]

@jeevank I started the work but was unable to work through a bunch of failing tests in the time frame I had - unfortunately, never got a chance to pick things back up. We would still like to make this happen, though!

[dotnetCarpenter]

@DonutEspresso Perhaps you could publish your work? I don't see anything at https://github.com/DonutEspresso/node-ecstatic/

[DonutEspresso]

@dotnetCarpenter I had the work sitting on a local branch on my old machine, but the HDD died over the winter break. :( I recall there wasn't a whole lot of work - it's more around debugging the odd corner case here and there (in particular, the error cases). Unfortunately it doesn't look like I'll get a chance to dig into this anytime soon.