feat(common): extend streamable-file header support

[davidschuette]

Provide option to specify additional headers when using StreamableFile.
No need to access the native response option.

BREAKING CHANGE: not specifying content-disposition header and
StreamableFile option disposition would send header value 'null'
instead of not sending the header at all.
This changes to not sending the header if no value is specified.

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Other... Please describe:

What is the current behavior?

Issue Number: #9229

What is the new behavior?

More headers can be specified when using the StreamableFile class without needing to access the native response object.

Does this PR introduce a breaking change?

• Yes
• No

BREAKING CHANGE: not specifying content-disposition header and
StreamableFile option disposition would send header value 'null'
instead of not sending the header at all.
This changes to not sending the header if no value is specified.

Other information

[coveralls]

Pull Request Test Coverage Report for Build c59e21a2-4a17-496f-a5c6-8c268288f992

• 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.03%) to 94.189%

---

Totals
Change from base Build 82baa335-ce7d-43f6-a7cd-d81418408f3d:	0.03%
Covered Lines:	5722
Relevant Lines:	6075

---

💛 - Coveralls

[micalevisk]

any chance to make this feat backwards compatible? 👀

[davidschuette]

any chance to make this feat backwards compatible? eyes

The (potentially) breaking change is more of a bug fix in my opinion, as I don't think sending 'null' as header value is a good practice. The rest of the changes is backwards compatible.

[davidschuette]

I found another problem with the current implementation of the StreamableFile. Returning a StreamableFile object when using the @Res() decorator in the specifiy controller function results in a timeout.
I have created a branch of my repo to show the problem.
The route /:id works fine (although the headers are missing for this demo), but /:id/broken times out. The only difference is the reference to the response object.

@ApiOperation({ summary: 'Download a file.' })
@Get(':id')
downloadFile(
  @Param('id') id: string,
  @Req() request: Request,
  // @Res() response: Response,
) {
  return this.appService.download(id, request)
}

@ApiOperation({ summary: 'Download a file.' })
@Get(':id/broken')
downloadFileBroken(
  @Param('id') id: string,
  @Req() request: Request,
  @Res() response: Response,
) {
  return this.appService.download(id, request)
}

As a result using the response object to define headers and the StreamableFile class for the stream is not an option.

[kamilmysliwiec]

Returning a StreamableFile object when using the @res() decorator in the specifiy controller function results in a timeout.

This is the expected behavior.

As a result using the response object to define headers and the StreamableFile class for the stream is not an option.

You shouldn't use both at the same time.

[davidschuette]

The (potentially) breaking change is more of a bug fix in my opinion, as I don't think sending 'null' as header value is a good practice. The rest of the changes is backwards compatible.

@kamilmysliwiec Do you have an opinion on that "problem"?

[davidschuette]

This is the expected behavior.

Ok.

You shouldn't use both at the same time.

I just looked it up again and tried with passthrough: true and it works.

[jmcdo29]

I think this looks good. If anyone wants to add additional headers to the response, from here they can use the @Res({ passthrough: true }) approach to get access to the response object and its headers

[kamilmysliwiec]

LGTM