Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Ensure that links to directories end in a slash. #27

wants to merge 1 commit into from

Conversation

geekmug
Copy link

@geekmug geekmug commented Jan 26, 2015

I use strict routing and missing slashes causes redirection.

@dougwilson dougwilson added the pr label Jan 26, 2015
@dougwilson
Copy link
Contributor

Thanks! Can you fix the tests and update all the types for have the trailing slashes instead of only the HTML view?

@geekmug
Copy link
Author

geekmug commented Jan 26, 2015

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 { name: file, stat: stats[i] } objects used in the html function, but that breaks the public API. Unfortunately, the html export uses extra information from the stat to do the "details" view, so it's not trivial to eliminate. However, it's extremely likely the 2nd stat calls will be cached, so it probably will have no performance impact

@dougwilson
Copy link
Contributor

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.

@geekmug
Copy link
Author

geekmug commented Jan 26, 2015

Err, sorry for the churn, caught a mistake stripping the slash off of ".." (which obviously doesn't make sense).

@geekmug
Copy link
Author

geekmug commented Jan 26, 2015

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.

@dougwilson
Copy link
Contributor

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 :)

@dougwilson dougwilson self-assigned this Jan 26, 2015
@geekmug
Copy link
Author

geekmug commented Jan 26, 2015

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 (req, res, files, next, dir, showUp, icons, path, view, template, stylesheet). And, the part I need is path.

@dougwilson
Copy link
Contributor

Feel free to add new arguments on the end :)

@andrepadez
Copy link

@geekmug , it seems to be working fine, except for one tiny detail:

@andrepadez
Copy link

Also, this feature needs to be added as an option, at least until the next major release. Checkout my pull request's #31 conversation.
Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants