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

feat(core): add file send capability #5278

Merged
merged 5 commits into from
Feb 1, 2021

Conversation

jmcdo29
Copy link
Member

@jmcdo29 jmcdo29 commented Aug 15, 2020

Fastify has the built in capbility to send files without
any extra code. Express can send files via stream.pipe(res)
if the file is a stream, and as such, the
Content-Type header is set to application/octet-stream
in the ExpressAdapter if a stream or Buffer is detected.

fix #5263

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[X] 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?

Files are not currently supported by Express and Fastify

Issue Number: #5263

What is the new behavior?

Fastify and Express Adapters now both support sending files via streams and Buffers.

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

@coveralls
Copy link

coveralls commented Aug 16, 2020

Pull Request Test Coverage Report for Build 0646adc6-5d7d-4efd-a4d6-05e8e76f2a14

  • 4 of 11 (36.36%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 94.621%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/common/file-stream/streamable-file.ts 2 9 22.22%
Totals Coverage Status
Change from base Build dc31867d-4641-4462-b640-88e523413104: -0.1%
Covered Lines: 5084
Relevant Lines: 5373

💛 - Coveralls

@kamilmysliwiec kamilmysliwiec added this to the 7.5.0 milestone Oct 5, 2020
@kamilmysliwiec kamilmysliwiec removed this from the 7.5.0 milestone Oct 29, 2020
@kamilmysliwiec
Copy link
Member

I've just realized that this may actually introduce a breaking change. Very rare case, but what if someone has a class that contains a custom pipe() method defined. Although this object is intended to be serialized as any other DTO, Nest would try to pipe a stream.

To avoid a breaking change, perhaps we can define a new, dedicated wrapper class that you could use as follows new ResponseStream(stream) (or any other, better name) and then just check with instanceof instead of checking for pipe method

@jmcdo29
Copy link
Member Author

jmcdo29 commented Oct 29, 2020

I've just realized that this may actually introduce a breaking change. Very rare case, but what if someone has a class that contains a custom pipe() method defined. Although this object is intended to be serialized as any other DTO, Nest would try to pipe a stream.

To avoid a breaking change, perhaps we can define a new, dedicated wrapper class that you could use as follows new ResponseStream(stream) (or any other, better name) and then just check with instanceof instead of checking for pipe method

Oh wow, very good catch, hadn't thought of that. As Fastify already handles streams and buffers, would this change only affect Express, or should we make a similar change (using this new class) for Fastify as well, to keep parity between the engines?

@kamilmysliwiec
Copy link
Member

As Fastify already handles streams and buffers, would this change only affect Express, or should we make a similar change (using this new class) for Fastify as well, to keep parity between the engines?

Well, I guess we can just add a new wrapper class and mention it in the docs as a "cross-platform" supported solution. As you pointed out above, since Fastify already handles streams and buffers, using this class won't be required (but still, it gives some ++ in case one wants to switch to another HTTP engine at some point).

@kamilmysliwiec kamilmysliwiec mentioned this pull request Nov 2, 2020
@jmcdo29
Copy link
Member Author

jmcdo29 commented Nov 28, 2020

Circling back around to this: what about StreamableFile as a class name? That or NestStreamable or NestFile? Something that specifies it's to be used with the framework.

Also, where in the code would this go? I assume @nestjs/common, but not sure where from there. http?

@kamilmysliwiec
Copy link
Member

StreamableFile sounds good :)

I assume @nestjs/common, but not sure where from there. HTTP?

We might need a new folder for this. I'm open to any ideas. classes seems to be somewhat too generic

@jmcdo29
Copy link
Member Author

jmcdo29 commented Dec 8, 2020

All right, I'll get back to work on this PR when I've got some time and we can discuss what changes might need to be made when I update the PR

@jmcdo29
Copy link
Member Author

jmcdo29 commented Dec 10, 2020

All right, added in the StreamableFile class and I think it's working as expected. Let me know what you think and if I should get to work on the docs.

@kamilmysliwiec
Copy link
Member

Looks great! @jmcdo29 can you please resolve conflicts + create a PR to the docs with this feature? I'm not sure what would be the best chapter to add it though (maybe FIle Upload although the name may be a little bit confusing)

@jmcdo29
Copy link
Member Author

jmcdo29 commented Jan 27, 2021

What if we add a section to the Controllers page about Streaming Files? Though, that page is getting pretty cluttered. Maybe a new Technique?

Fastify has the built in capbility to send files without
any extra code. Express can send files via `stream.pipe(res)`
if the file is a stream, and as such, the
`Content-Type` header is set to `application/octet-stream`
in the ExpressAdapter if a stream or Buffer is detected.

fix nestjs#5263
The new `StreamableFile` class allows for users to create
an easy to consume file to send back to the client side. Both
`ExpressAdapter` and `FastifyAdapter` have been updated to
accomodate for this new class.
@jmcdo29
Copy link
Member Author

jmcdo29 commented Jan 28, 2021

Merge conflicts are fixed. Kafka test failed (as usual)

@kamilmysliwiec
Copy link
Member

LGTM, thanks!

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.

Sending Files
3 participants