res.download and dotfiles

[CalogeroMandracchia]

res.sendFile() lets you set dotfiles ( e.g. {dotfiles: 'allow'} ) while we don't have this option for res.download.
Is there a way to set it?

[dougwilson]

Hi @CalogeroMandracchia there is no way to set it currently. You're welcome to make a pull request to add this feature :) ! If you want to, and not sure where to start, let us know and we can help you.

[chillypepper]

Hey @dougwilson, taking you up on that good first contribution tag :) I've tried my hand with pull request #3370, and just had a couple of notes I thought I'd add here. I found this whole process unexpectedly nerve wracking and keep worrying I'll have missed something obvious, so please have mercy if I did hahaha.

I've added an options parameter to the function that just passes through to sendFile, so dotfiles and anything else should work properly here. I'm not exactly sure how you'd like to handle the possibility of header conflict though - if a user passes in their own custom headers, should they completely overwrite the default header (which just sets the Content-Disposition), or should they merge and only prioritise the provided Content-Disposition if it's explicit?

For now I've assumed that the entire header field should overwrite the default, because it's always pretty ugly having to remove an added default value (unless you make people explicitly set it to null if they don't want the field?). I've added tests for a blank options field, one with dotfiles provided, and one overriding the default Content-Disposition - should I add any more tests for this? Running npm test passes, and it all seems to be working properly, but if there's anything wrong with styles/formatting/methodology or even a case I've missed let me know and I'll fix it up. I copied the res.sendFile method for handling an additional parameter so hoping that's okay.

Also just wanted to say great work on expressjs - I've only been using it for a couple of months now and absolutely loving it, so thank you :)

[dougwilson]

Hi @chillypepper this is great, really appreciated! Funny that it sounds like you were worried about what turns out to be the one issue with the PR :) I would definitely change the implementation to one of the following:

1. Ignore the headers option
2. Merge the headers, where the generated content-disposition overwrites whatever the user gave.

The reason is that if the user supplies the content-disposition in the current implementatiom, it will work identically to res.sendfile , which seems pointless, as they should just directlt call res.sendfile if they want to specify that header manually :)

[chillypepper]

Hey @dougwilson, thank you, I'm so glad to hear that :) Dang, I was so close hahaha, one issue I can live with though. I pushed up a new commit a few seconds ago with a mis-implemented piece (I had thought you said allow the content-disposition to be overwritten), and just about to upload another now that follows option 2, and allows headers to be included while overriding Content-Disposition.

That makes sense given download's purpose so I've included a note in the function documentation saying to just use res.sendFile directly if you don't want the custom Content-Disposition, just in case someone doesn't expect it. I've also fixed up the test cases to test merging with a custom header, and making sure the Content-Disposition can not be overridden.

Hopefully this will cover anything now - thanks for being so patient with me on this, hopefully it will all work properly now.

[Mrashes]

Is this issue still a problem, I can help out if needed but would need some guidance as I don't often work on open source projects.

[dougwilson]

Looks like thr PR submitted for this was never linked back here, so there is the link: #3370

It will be included in thr 4.16 release, but if you want to try it out before hand, that would out as extra eyes to make sure it'll be the new feature you are looking for.

[dougwilson]

I fixed up the labels on here as well.