res.sendFile: inherit etag setting from app #2936
Conversation
This reverts commit 21db675c7e149fb858810f85989bd9f6512335e0.
lib/response.js
Outdated
| @@ -368,6 +371,10 @@ res.sendFile = function sendFile(path, options, callback) { | |||
| done = options; | |||
| opts = {}; | |||
| } | |||
|
|
|||
| 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
| @@ -368,6 +369,10 @@ res.sendFile = function sendFile(path, options, callback) { | |||
| done = options; | |||
| 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
| @@ -368,6 +369,10 @@ res.sendFile = function sendFile(path, options, callback) { | |||
| done = options; | |||
| opts = {}; | |||
| } | |||
|
|
|||
| 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. |
|
Any updates with this PR? |
|
Most likely outdated |
|
I'm not seeing this merged |
|
@Doc999tor You're right, this was never merged. I haven't been following development of expressjs for a few years. If you want you can finish this PR. Maybe first check if the project is still being maintained. |
feature mentioned in: #2294
If ETag is disabled in app settings, res.sendFile inherits this setting in options object