search

expressjs / serve-static

Serve static files
MIT License
1.39k stars 228 forks source link

Feat document link to etag through send #46

Closed madarche closed 8 years ago

madarche commented 9 years ago

This explains the etag dependency link which is not directly present in the code of this module. It eases code browsing.

madarche commented 8 years ago

@dougwilson could you consider this PR please? This is really an easy one to accept :-)

Cheers

dougwilson commented 8 years ago

Hi @madarche , sorry, there are so many issues flooding in recently and I'm also on vacation for the last 2 weeks here I cannot keep up. But since you pinged me, I will take some time here out of my vacation to follow up on this PR :)

In your update, you are adding a new paragraph for the "etag" option with the text "For more information, see their documentations.". I don't understand exactly what more information people will understand when they follow those links in regards to the "etag" option for this module, can you elaborate on exactly what in those links users need? Can whatever it is simply be included here and not linked?

You'll notice that most of this README is similar to the send readme and this is on purpose: we document everything in full here and don't provide links to other APIs that just confuse users. For example, how does understanding the etag module's API help with the etag option? All you can specify is true/false here.

madarche commented 8 years ago

@dougwilson very sorry to disturb you during your vacation! I had checked your "contribution activity" timeline but since you're contributing-all-the-time, your vacation is quite difficult to spot ;-) (maybe changing your name in your profile to something like "Douglas Christopher Wilson on vacation" … ;-) And only answer to this message WHEN you want/can.

Anyway the story is that I was trying to spot the etag dependency in serve-static (while working on the Express documentation). And with a grep in a newly cloned serve-static I could only find the etag string in the README.md and HISTORY.md files. Nothing about etag in the code, that is index.js, neither in package.json. So I was really wondering where the etag generation was coming from and I have even thought that the README.md was lying by being out of date.

But now I have read your comment I totally understand the purpose. My point is to ease navigating through the code and modules to easily find how it works (to easily fix things, etc.). So I'm totally OK to reject/revert/whatever my proposition and replace it with something like:

Under the hood serve-static uses the send module and thus leverages all its functionality (etag, etc.). For more information, see its documentation.

What do you think? If you're OK shall I do that in the same PR (that will include a revert) or should I create a new PR?

Thanks a lot!

dougwilson commented 8 years ago

Anyway the story is that I was trying to spot the etag dependency in serve-static (while working on the Express documentation). And with a grep in a newly cloned serve-static I could only find the etag string in the README.md and HISTORY.md files. Nothing about etag in the code, that is index.js, neither in package.json. So I was really wondering where the etag generation was coming from and I have even thought that the README.md was lying by being out of date.

Ok, but what was the purpose of the search? Was it because the documentation regarding the "etag" option in the README was not good enough and just needs to be expanded? There are many, many ways to write code such that even if this module implemented the etag generated, a grep for "etag" could turn up nothing.

My point is to ease navigating through the code and modules to easily find how it works (to easily fix things, etc.).

That's fine. The purpose of the npm site (https://www.npmjs.com/package/serve-static) is to list dependency graphs, rather than manually maintaining one in the README (and a programmer can examine the package.json file). Typically, I have never had a programmer have an issue locating the etag dependency to fix issues or determine how it works, because the code is very straight-forward:

  1. options comes in to https://github.com/expressjs/serve-static/blob/v1.10.0/index.js#L37
  2. Copied to opts at https://github.com/expressjs/serve-static/blob/v1.10.0/index.js#L47
  3. The opts variable is passed to send at https://github.com/expressjs/serve-static/blob/v1.10.0/index.js#L95

And that's the end of the search: anything not in here is passed right on through to the send function, which comes from https://github.com/expressjs/serve-static/blob/v1.10.0/index.js#L19 .

The goal of the README is to help people understand using the module and not be confusing. Developers do not have any issues following the code around, and a lot will even simply step through the code with a debugger instead of following it by hand, to determine where a bug is at.

So I'm totally OK to reject/revert/whatever my proposition and replace it with something like:

Under the hood serve-static uses the send module and thus leverages all its functionality (etag, etc.). For more information, see its documentation.

The biggest problem with this is that now is couples this module to the send module, which is not desired. It means that it becomes that much harder to just swap the entire dependency randomly, perhaps because there is a new one that's better, or perhaps a design flaw resulting in an un-fixable security issue. As far as the README should be concerned, the full public API should be documented, not any private APIs (the internal details of dependencies are not public APIs and just won't be included on the README).

The exception to this is in the https://github.com/expressjs/serve-static#maxage section, with the link to the ms module, which I really hate. Any necessary documentation from that module really needs to be added to this README and the link removed, but I haven't had time for that since it's an enormous documentation task (because of all the units it supports).

What do you think? If you're OK shall I do that in the same PR (that will include a revert) or should I create a new PR?

I'm' OK with something you think will help, as long as it's not a link/mention of the send module in the README (perhaps a comment in the source code may help here? I'm not sure, because the flow of determining where the etag option goes is so straight-forward in the code).

madarche commented 8 years ago

Hello @dougwilson . It's me taking very very long time to answer this time. Sorry. Your answer was so precise I wanted to meet the requirements. I fully agree with all what you've written.

The purpose of my search was, as a developer, to check how the etag computation was implemented in serve-static while investigating for an issue.

The new state of this PR is my last proposal. If it doesn't satisfy you let's just forget about it. And thanks for all the time you've spent on this.

madarche commented 8 years ago

51 proposes to remove tests against Nodejs 0.8 which makes the CI fails for this PR simply consisting of a comment in the source. So @dougwilson please consider accepting #51 before accepting #46. Thanks.

madarche commented 8 years ago

Understood. Thanks for your time. Let's then close this PR.