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

added sendFile #36

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kylegoetz
Copy link
Contributor

I wanted to be able to send files with hyper-ts but there was no way to use it natively. I had to write Express code and then use fromRequestHandler to turn it into a hyper-ts middleware.

One can use sendFile it in lieu of send. It will also transition the index from BodyOpen to ResponseEnded. It takes an error handler just like json does, and it will trigger the error handler if one of three things is true:

  1. the path is a relative path (Express, for example, only allows absolute paths, so this is somewhat opinionated); I've also named the parameter in sendFile absolutePath to hint to an end user that the path must be absolute.
  2. the path is to a file, not a directory
  3. the file exists

I updated the ExpressConnection to work with this as well and added some unit tests.

@kylegoetz
Copy link
Contributor Author

kylegoetz commented Dec 28, 2020

Also I started to add a @since 0.7 but wasn't sure if I should. I figure if the PR is accepted, a maintainer would correct that. I did not commit the updated documentation for the same reason.

@kylegoetz
Copy link
Contributor Author

kylegoetz commented Dec 28, 2020

One thing to address: should sendFile attempt to auto-detect mime type and add the appropriate Content-Type header? I have not done this because the most straightforward way to do this would be to add another dependency that does this (there are so many mime types!). I didn't want to force a dependency.

I suppose sendFile could take an optional Content-Type parameter. Might that be the right way to do it? Or should we just expect the user to explicitly chain addHeader('Content-Type', 'application/pdf') for example? In my own project, this is what I'm doing.

@kylegoetz kylegoetz mentioned this pull request Dec 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant