support filepaths to .contentType()

[stephenplusplus]

I noticed the readme makes it seem as though file paths will please the .contentType() method, but when used with a path like "here/is/my/file.json", the directory separator / is being mistaken for a content-type's /, which is interpreted as "I don't need you to lookup the content-type". Instead of looking for a /, I changed it look for a ;. Hope this isn't a terrible solution!

[dougwilson]

So, unlike a /, a ; character can actually be in a file name, so changing the detection to be ; instead of / is probably worse. It also makes providing mime types not even work, since a mime type like text/html doesn't even contain a ; character.

Simply do this in your code, though:

mime.contentType(path.basename('path/to/file.json'))

[stephenplusplus]

Sure, I just thought it'd be a nice API to save the steps. Regarding breaking mime types, judging from the tests, this change was backwards compatible in that regard.

As far as I can tell, the line that checks for the presence of the / is to cater to:

assert.equal(contentType('text/html; charset=ascii'), 'text/html; charset=ascii')

If I'm not overlooking anything, I think it makes for a more useful API to cater to:

assert.equal(contentType('/path/to/file.html'), 'text/html; charset=UTF-8')

Since in the first case, I don't need the method to generate a content-type header value, while in the second, I do. Is there a way we haven't thought of yet that could support both use cases?

Thanks for the consideration!

[dougwilson]

Ah. Can you tell me what will happen to the people currently doing contentType('some;file.html'), since ; is a valid character in a file name, unlike /? How does the result of that compare to before the changes here (sorry, I'm mobile right now)?

[stephenplusplus]

They'd be busted by the new check for ;. I didn't realize ; was a valid character in a file name when I added that.

I pushed a new commit that would end support for .contentType() accepting content-types. As explained above, I'd rather use this method to get a content type back from a file path or file type. However, if there is a safe solution to detect when a content type is provided, it would be even better to continue to support all use cases.

[dougwilson]

Cool. So that would bump this PR to a new major. It's certainly doable, but there wouldn't be a timeline on that, unfortunately.

In reality, our intention was actually to remove contentType in the next major anyway, and possible resurrect set-type to take over.

[jonathanong]

yeah i ran into this issue sometimes. sometimes i pass a filename, other times i pass a mime type, but i expect both to work.

[jonathanong]

@dougwilson we could also use the content-type module and have contentType.fromFilename(), contentType.set(), contentType.parse(), etc.

[dougwilson]

Hm, yea, maybe, though I wasn't intending to have a dep on mime-* in that lib, IDK

[dougwilson]

As long as .contentType() supports an actual MIME type, we really cannot add more ambiguity here. Just think you want to set your content type to the type application/vnd.apache.thrift.json ... we cannot tell what you want to do.

[dougwilson]

I still need to revive the set-type lib, but I don't see a good reason not to just use path.extname. I added an example to the README for this.

[stephenplusplus]

The only reason I thought this was supported in the first place was the second example in the readme (https://github.com/jshttp/mime-types/blob/master/README.md#mimecontenttypetype):

mime.contentType('file.json') // 'application/json; charset=utf-8'

When I saw that, I thought "that makes so much sense!" That's my most common use case. I don't know why getting back what you give it is a more common one, but regardless, I'll continue to be a happy user of the library. Thanks!

[dougwilson]

Right. This is one reason I don't particularly like functions that behave differently depending on their argument. Ideally we would have a CT function that takes a type and a different CT function that takes file paths.