res.sendFile: inherit etag setting from app#2936
res.sendFile: inherit etag setting from app#2936hrvolapeter wants to merge 10 commits intoexpressjs:5.0from hrvolapeter:5.0
Conversation
This reverts commit 21db675c7e149fb858810f85989bd9f6512335e0.
lib/response.js
Outdated
| } | ||
|
|
||
| if (app.settings.etag == false) { | ||
| opts.etag = app.settings.etag; |
There was a problem hiding this comment.
I would expect that if the user explicitly set the etag option to res.sendFile, it would not be overwritten by the setting.
|
@dougwilson I wrote those test and some of they are failing but I think it never worked, as the external package used for sending files allow only setting etag to true | false. I will take a closer look at the problem. |
|
The way those packages are, you cannot make them send strong ETags, because they have no way to make the guarantees required for an ETag to be strong. I'm not saying those options need to be supported, only that the behavior needs to be tested. |
lib/response.js
Outdated
| opts = {}; | ||
| } | ||
|
|
||
| if(! ('etag' in opts) ) { |
There was a problem hiding this comment.
Pleas do not use in for option detection. This makes the interface really confusing because you have to ensure that whatever you are passing in does not have that key anywhere in the prototype chain.
lib/response.js
Outdated
| } | ||
|
|
||
| if(typeof opts.etag == 'undefined') { | ||
| opts.etag = app.set('etag'); |
There was a problem hiding this comment.
Do not pass in non-booleans to the sub module, because that is just asking for undefined behavior later. The etag option only accepts true or false.
|
This PR has been updated. Github doesn't send notifications out automatically for this type of change, so this is just a comment to generate a notification. |
4 participants
feature mentioned in: #2294
If ETag is disabled in app settings, res.sendFile inherits this setting in options object