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

Request for request #3327, allowing res.download to supply options to res.sendFile #3370

Closed
wants to merge 3 commits into from
Closed

Request for request #3327, allowing res.download to supply options to res.sendFile #3370

wants to merge 3 commits into from

Conversation

chillypepper
Copy link
Contributor

  • Added an "options" field to res.download, allowing for options to be passed into sendFile
  • Options will include the existing Content-Disposition header by default, unless explicit headers are provided
  • Added tests for the new res.download functionality - with blank options, with allowed dotfiles, and with a custom Content-Disposition

…e passed into sendFile

- Options will include the existing Content-Disposition header by default, unless explicit headers are provided
- Added tests for the new res.download functionality - with blank options, with allowed dotfiles, and with a custom Content-Disposition
Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe just the one nit I mentioned in the original issue thread :) Thanks for you hard work on this!

… rather than be overriden by them

- Included a note in the function docs recommending res.sendFile if this behaviour doesn't meet the use case
- Modififed a test for res.download to check for the existence of merged headers
… provided Content-Disposition will be overridden

- Function docs and test case for provided Content-Disposition have been updated to reflect this
@chillypepper
Copy link
Contributor Author

I've chosen to follow option 2 from the thread, "Merge the headers, where the generated content-disposition overwrites whatever the user gave." Ignore that quick commit in the middle hahaha, it should all work nicely now. Thanks for your help with this!

Copy link
Member

@LinusU LinusU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat 👍

@dougwilson dougwilson self-assigned this Sep 28, 2017
@dougwilson dougwilson added this to the 4.16 milestone Sep 28, 2017
@dougwilson dougwilson mentioned this pull request Sep 28, 2017
20 tasks
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