Add ability for developers to support additional MIME types in FileMiddleware #657
Conversation
|
I actually think it's a good case for making that property explicitly mutable. I don't see any reason we wouldn't do so. While adding a function to modify those properties is nice, it's certainly also no harm and potentially helpful to know all registered file extensions. |
|
What do you think @adam-fowler ? |
The main reason I took this approach was because it isn't immediately obvious that the I guess I don't have a strong feeling either way to make it mutable or not, but as it is, future maintainers would have the explicit context of associating the file extension with the mime type instead of having to intuit the association. |
I could also make it public, but keep it immutable. :) |
|
That makes sense! |
So, leave it as it is, but make the property public? |
I don't think it's a great idea to make what should be a static list (you aren't going to change this while the server is running) mutable. |
| urlBasePath: urlBasePath, | ||
| cacheControl: cacheControl, | ||
| searchForIndexHtml: searchForIndexHtml, | ||
| additionalMediaTypeExtensions: [:] |
There was a problem hiding this comment.
Of should it be the other way fileExtensionMediaTypeMap? What do you think
There was a problem hiding this comment.
I think they are about equal in terms of clarity, but the first is more concise.
There was a problem hiding this comment.
I think they are about equal in terms of clarity, but the first is more concise.
I just realized they are the same character count, but the second looks longer lol
There was a problem hiding this comment.
Just pushed - I also changed
public func withAdditionalMediaType(_ mediaType: MediaType, forFileExtension fileExtension: String) -> FileMiddleware
to
public func withAdditionalMediaType(_ mediaType: MediaType, mappedToFileExtension fileExtension: String) -> FileMiddleware
to match.
| @@ -52,6 +52,7 @@ where Provider.FileAttributes: FileMiddlewareFileAttributes { | |||
| let searchForIndexHtml: Bool | |||
| let urlBasePath: String? | |||
| let fileProvider: Provider | |||
| public let mediaTypeFileExtensionMap: [String: MediaType] | |||
There was a problem hiding this comment.
Is there a reason this needs to be public.
There was a problem hiding this comment.
re @Joannis
it's certainly also no harm and potentially helpful to know all registered file extensions.
I thought it was a good point. I could see a need to introspect what the customizations are.
There was a problem hiding this comment.
Until someone needs it I'd prefer we leave it private. Setting it to public means setting it in stone until the next major version.
| self.mediaTypeFileExtensionMap = mediaTypeFileExtensionMap | ||
| } | ||
|
|
||
| public func withAdditionalMediaType(_ mediaType: MediaType, mappedToFileExtension fileExtension: String) -> FileMiddleware { |
There was a problem hiding this comment.
Is it worthwhile adding a function to add multiple media types.
public func withAdditionalMediaTypes(_ types: [String: MediaType]) -> FileMiddleware
There was a problem hiding this comment.
Here are sample call sites to help visualize the difference:
FileMiddleware(".")
.withAdditionalMediaType(hlsStream, mappedToFileExtension: "m3u8")
.withAdditionalMediaType(MediaType(type: .application, subType: "foo"), mappedToFileExtension: "foo")
.withAdditionalMediaType(MediaType(type: .application, subType: "bar"), mappedToFileExtension: "bar")
.withAdditionalMediaType(MediaType(type: .application, subType: "baz"), mappedToFileExtension: "baz")FileMiddleware(".")
.withAdditionalMediaTypes([
"m3u8": hlsStream,
"foo": MediaType(type: .application, subType: "foo"),
"bar": MediaType(type: .application, subType: "bar"),
"baz": MediaType(type: .application, subType: "baz"),
])I definitely like the less imposing block of text, but it still retains the original issue I was trying to avoid in that it's not super apparent that the keys in the dictionary are file extensions.
...future maintainers would have the explicit context of associating the file extension with the mime type instead of having to intuit the association.
What would you think of
public func withAdditionalMediaTypes(forFileExtensions extensionToMediaTypeMap: [String: MediaType]) -> FileMiddleware
|
I looked at the details for the validation failure and it isn't making sense to me. Does it want me to format like this?!?! let extensions =
extensionToMediaTypeMap
.reduce(into: mediaTypeFileExtensionMap) {
$0[$1.key.lowercased()] = $1.value
} |
| withAdditionalMediaTypes(forFileExtensions: [fileExtension: mediaType]) | ||
| } | ||
|
|
||
| public func withAdditionalMediaTypes(forFileExtensions extensionToMediaTypeMap: [String: MediaType]) -> FileMiddleware { |
There was a problem hiding this comment.
@adam-fowler Is it worth adding a custom ExpressibleByDictionaryLiteral type for this dictionary use case? That way we can represent it in, what I believe, would be a clearer API surface. That could also cascade into the public var extensionMap: ... { get } down the line
There was a problem hiding this comment.
If we did, that would also allow us to account for non lowercase file extensions through the custom type, superseding the need for #661
Added
additionalMediaTypeExtensionstoFileMiddlewarewith awithAdditionalMediaTypemethod, adding additional media types, to retain object immutability.I also went out on a limb and funneled the
FileMiddlewareinits through a single initializer as that better supports this pattern.This would resolve #656
A test case was added to confirm expected behavior.