Promote SendFile extension methods to HttpResponse #544
Conversation
| } | ||
|
|
||
| // Not safe for overlapped writes. | ||
| private static async Task SendFileAsync(Stream outputStream, string fileName, long offset, long? length, CancellationToken cancellationToken) |
There was a problem hiding this comment.
Do we still want this fallback?
khellang
force-pushed
the
send-file
branch
2 times, most recently
from
c5276ac to
ba3ece9
Compare
|
I'm not sure this should be on HttpResponse but I'll let others chime in |
khellang
force-pushed
the
send-file
branch
3 times, most recently
from
9ac0093 to
3cdcf78
Compare
| @@ -11,11 +11,13 @@ | |||
| }, | |||
| "dependencies": { | |||
| "Microsoft.AspNetCore.Http.Features": "1.0.0-*", | |||
| "Microsoft.Extensions.FileProviders.Abstractions": "1.0.0-*", | |||
There was a problem hiding this comment.
HttpAbstractions picking up a dependency on FileProviders is pretty strange.
There was a problem hiding this comment.
It's not just FileProviders, it's FileProviders.Abstractions. In my head, it made sense that one abstraction package depends on another 😄
|
👎 We don't even let WriteAsync(string) live directly on HttpResponse. SendFileAsync is even less common. The IFileInfo overloads are interesting, but they belong in Extensions, not Abstractions, especially since they come with an added FileProviders dependency. |
khellang
force-pushed
the
send-file
branch
2 times, most recently
from
bb0ab67 to
6f17912
Compare
|
+1 I think extension method + IFileInfo is good enough |
|
closing this |
#527
I think we can remove some code duplication in after this as well:
https://github.com/aspnet/StaticFiles/blob/dev/src/Microsoft.AspNetCore.StaticFiles/StaticFileContext.cs#L379-L397
https://github.com/aspnet/StaticFiles/blob/dev/src/Microsoft.AspNetCore.StaticFiles/StaticFileContext.cs#L331-L346