-
-
Notifications
You must be signed in to change notification settings - Fork 151
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
Ensure that links to directories end in a slash. #27
base: master
Are you sure you want to change the base?
Conversation
Thanks! Can you fix the tests and update all the types for have the trailing slashes instead of only the HTML view? |
Whoops, sorry I spaced on the test cases. Originally, I was reluctant to change the applicaiton/json and text/plain output because of a fear of ambiguity if there was a path separator at the end of the file name (since in the HTML output a path separator in the file name would be encode into "%2F"), but I think it's fair to say that would be a very broken directory (I don't think POSIX even allows that). The other output types previously didn't call fs.stat for the directory contents, so I pulled that up into the main serveIndex function. As a side-effect of that, the html output now calls stat twice on each file in the directory. I wanted to change the "files" argument to be the |
Gotcha. I'm' sure you can refactor this some more to make sure there is only one stat call per file for even the HTML view. We also cannot add the post slash before calling the view functions, unless you're OK with this waiting until the next major version. |
Err, sorry for the churn, caught a mistake stripping the slash off of ".." (which obviously doesn't make sense). |
Ah ok, I understand the viewpoint that adding the slash broke the public API too. I can push the stat calls down into the exports.json and exports.plain, and that makes it only a single stat call per file too. If you that works for you, I'll refactor it that way. |
Yes, please :) In fact, I wanted to do this a long time ago, but didn't want to add the stat calls into JSON and text. Since you actually want it, I'm OK with it, since there's an actual demand now :) |
Is it a problem to add arguments to the json and plain functions? The path isn't passed down to those two functions, and there is no way to recover that information from within those functions (because they don't know the root either). It would be nice if they could have the same arguments as the html one, but that currently takes |
Feel free to add new arguments on the end :) |
@geekmug , it seems to be working fine, except for one tiny detail:
|
Also, this feature needs to be added as an option, at least until the next major release. Checkout my pull request's #31 conversation. |
I use strict routing and missing slashes causes redirection.