-
Notifications
You must be signed in to change notification settings - Fork 982
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
Issue 1604: Fix 'appendRequestPath: false' scenarios for Restify.plugins.serverStatic #1860
base: master
Are you sure you want to change the base?
Issue 1604: Fix 'appendRequestPath: false' scenarios for Restify.plugins.serverStatic #1860
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this thoughtful contribution 🙌
); | ||
var fileName = decodeURIComponent(path.basename(req.path())); | ||
// test for requested file, if it doesn't exist we send the directory as path since appendRequestPath = false | ||
if (!fs.existsSync(path.join(opts.directory, fileName))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: mind breaking these nested function invocations up into multiple lines w/ variables indicating what each path is supposed to represent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! let me work on it and I'll update the PR! great catch & suggestion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! I noticed the Issue #1604 is still open and has gotten some activity recently from people saying the issue is still unfixed. Are there any plans to submit this PR, or would you prefer if someone else creates a clone of this PR and submits that? - though that seems like unnecessary duplicate effort
Just for completeness, we also happen to have another flavor of serving static files with restify which has it's API syntax (and functionality) similar to that of expressjs. We had seen a lot of issues with Here is a small example of how to use it, feel free to play around with it: https://codesandbox.io/s/gallant-raman-9x9jk?file=/app.js |
Pre-Submission Checklist
make prepush
Issues
Closes:
Restify.plugins.serverStatic option "appendRequestPath" is not working properly.
According to documentation, when appendRequestPath = false, the file we are requesting should be served directly from
directory
, this wasn't the case. for example:if we make a call to
/endpoint
it will attempt to serve __dirname/endpoint/index.html, instead of __dirname/index.html (because we are settingappendRequestPath: false
another example is that if we call
/endpoint/myCoolResource.json
it will also attempt to serve __dirname/endpoint/myCoolResource.json instead of __dirname/myCoolResource.jsonChanges
With this PR when the user is setting appendRequestPath = false, the server will serve the requested resource specified in the endpoint (or the default static resource) which is located directly on
directory
.Now the behavior adequate with what's described on http://restify.com/docs/plugins-api/#servestatic
Also we are modifying the function
testNoAppendPath
used by the tests which were passing as false positives so it catches these errors in the future. Also, we are adding more tests to increase the coverage of Restify.plugins.serverStatic function.