Closed stephenplusplus closed 9 years ago
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'))
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!
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)?
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.
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.
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.
@dougwilson we could also use the content-type
module and have contentType.fromFilename()
, contentType.set()
, contentType.parse()
, etc.
Hm, yea, maybe, though I wasn't intending to have a dep on mime-*
in that lib, IDK
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.
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.
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!
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.
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!