feat(plugin): plugin to serve static files #1753
Conversation
rajatkumar
requested review from
hekike,
retrohacker,
misterdjules and
DonutEspresso
rajatkumar
changed the title
|
|
||
| // when `send` encounters any `error`, we have the opportunity | ||
| // to handle the errors here | ||
| stream.on('error', function handleError(err) { |
There was a problem hiding this comment.
how about:
stream.on('error', function handleError(err) {
// When file does not exist
if (err.statusCode === 404) {
return next(new ResourceNotFoundError(requestedFullPath));
}
// or action is forbidden (like requesting a directory)
return next(new NotAuthorizedError(requestedFullPath));
});
There was a problem hiding this comment.
will it be always NotAuthorizedError ?
There was a problem hiding this comment.
I wanted to put down explicit cases and hence the if/else structure. I foresee we might have other cases(which we find later on as people start using this plugin) where we want to send explicit errors.
If it is fine, I would like to keep the current structure.
| @@ -0,0 +1,131 @@ | |||
| 'use strict'; | |||
| var assert = require('assert-plus'); | |||
There was a problem hiding this comment.
If this plugin is for new versions only, maybe we can use let/const here?
There was a problem hiding this comment.
I did not want to tackle the eslint rule that disables usage of es6 constructs in code as part of this PR. (Refer: https://github.com/restify/node-restify/blob/master/.eslintrc.js#L12). Let's keep that for a future PR where we might convert vars -> consts/let in the codebase.
| * @public | ||
| * @function serveStaticFiles | ||
| * @param {String} directory - the directory to serve files from | ||
| * @param {Object} opts - an options object, which is optional |
There was a problem hiding this comment.
Is it worth calling this out as an options object meant explicitly for send? I think we do something similar with other plugins like mime.
There was a problem hiding this comment.
The options object is not explicitly for send, I added another functionality (similar to express) that allows us to set custom headers by passing a function under options.setHeaders. eg.
setHeaders: function setCustomHeaders(response, requestedPath, stat) {
response.setHeader('restify-plugin-x', 'awesome');
}
})
Also, we explicitly override options.root before passing to send. So the other options might be similar to what send needs but they are not explicitly for send.
There was a problem hiding this comment.
Cool, makes sense. This might be paranoia but if it's not too expensive, you could think about making a new copy of opts when we call send but with only send specific options. It's nice to be able to separate internal opts from send opts. At the minimum - worth calling out in the jsdocs what's used by us vs used by send.
Pre-Submission Checklist
make prepushIssues
Closes:
Changes
Added the
serveStaticFilesplugin and set of tests. Also updates the docs to reflect this new plugin and its API.I decided to keep the existing plugin along with this one so that we are not breaking any existing usage of the
staticplugin in the ecosystem.