Skip to content

Added a new method to get all extensions.#26

Open
thihara wants to merge 7 commits into
jshttp:masterfrom
thihara:patch-1
Open

Added a new method to get all extensions.#26
thihara wants to merge 7 commits into
jshttp:masterfrom
thihara:patch-1

Conversation

@thihara

@thihara thihara commented Oct 15, 2016

Copy link
Copy Markdown

Added a new method called 'allExtensions' to get all the extensions matching a given mime-type.

Added a new method called 'allExtensions' to get all the extensions matching a given mime-type.
Added information about how to use the allExtensions method.

@dougwilson dougwilson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for contributing, @thihara ! It looks like a few things:

  1. Can you include a little bit of docs in the PR?
  2. Can you add some tests to our test suite to cover the new functionality?
  3. If you can run npm run lint to lint the code and fix the linting issues, that would help a lot :) We use the Standard JS style (http://standardjs.com/) which, namely, requires no semicolons.

Thank you so much for helping out!

@thihara

thihara commented Oct 15, 2016

Copy link
Copy Markdown
Author

@dougwilson Done. There was an issue with the eslint command, it was ignoring the index.js file. Please resolve the conflict and merge.

Comment thread README.md Outdated
@dougwilson

Copy link
Copy Markdown
Contributor

Awesome, thanks, @thihara ! The GitHub web interface does not provide any method to resolve conflicts, so I will not be able to merge until I'm at a computer (vs being on my phone), so it may not be until tomorrow at the earliest that I can merge with a conflict :) Also, I noticed a typo in your docs. If you can fix that, that would be awesome, otherwise I'll get that resolved tomorrow or whenever I'm at a computer again for ya :)

@thihara

thihara commented Oct 15, 2016

Copy link
Copy Markdown
Author

@dougwilson I think you meant the missing 's' at the allExtension. I fixed that. Thanks. I'm not sure if I can resolve the conflict myself without write access.

@dougwilson

Copy link
Copy Markdown
Contributor

So since I don't have much to do on my phone, I was reading back up on this issue to see why it wasn't gotten to yet, and now I remember: this is because the current array of extensions is actually completely useless save for the first element in the array. Going back to koajs/koa#617 (comment) remined me on what the blocker to adding a real API to this module was, which was to actually get that array to be in a meaningful order for the use-cases that wanted to use the functionality that I was aware of.

What is the use-case you are trying to use this function for? Is it similar to that Koa one, or something different?

@dougwilson

Copy link
Copy Markdown
Contributor

I'm not sure if I can resolve the conflict myself without write access.

To resolve the conflict from your side you either need to simply remove the conflicting change from your PR (which is what I was going to do anyway--the package.json change on master is the one I will be keeping over the change here) or you can rebase your branch on top of the current master, which would force you to go through the resolve process and then force push an update to your branch.

Anyway, I can resolve the merge, but will just have to wait until I'm at a computer at the earliest, since the GitHub web interface provides no method to resolve conflicts (the merge button on this PR is just literally disabled with no action I can take to merge or resolve).

But in addition, I was reading back over the original issues, which brought up a question I posted in the previous comment if you want to discuss while I'm around right now :)

@dougwilson

Copy link
Copy Markdown
Contributor

I think you meant the missing 's' at the allExtension. I fixed that. Thanks.

haha, I didn't even notice that. What I saw was the example code had the function name of just "extension" rather than "allExtension". It's hard to review anything from a phone, I guess :) Small screen and scrolling through source code is not that great of an experience, haha.

@thihara

thihara commented Oct 15, 2016

Copy link
Copy Markdown
Author

@dougwilson Perhaps, but what I wanted to get was a list of possible extensions for a mime-type. This is just for some validations. Nothing like the issue you mentioned in koa.

Ah I think I'll let you do the merge in that case 😆

@dougwilson

Copy link
Copy Markdown
Contributor

Cool, will do :) So I guess I have nothing else to do what research around this issue :) One thing this module does is provide compatible APIs with the mime npm module. I then wondered: has that module added a similar API? It turns out no, but there is a PR to add this exact thing to that module: broofa/mime#136

We should probably coordinate to make sure that we both end up landing a function with the same name. If you want to take point on that, that'd be awesome! Otherwise, I'll go poke on that PR to see if we can get alignment on the function name prior to landing this.

@thihara

thihara commented Oct 15, 2016

Copy link
Copy Markdown
Author

Good luck with that. The PR in question seem to be open for months without a response. 💃
I'd consider that a dead repo for all intents and purposes.

mime-types is reverse compatible for all existing methods of node-mime and since no activity seem to be happening in that repo, I see no point in trying to be completely inter-operable. Specially for a function that doesn't exist in there yet.

Reverted lint command changes. Better one is available now in master branch.
@thihara

thihara commented Oct 16, 2016

Copy link
Copy Markdown
Author

@dougwilson I managed to remove the changes in package.json, since you said you will be discarding them anyway. Looks like we have a PR without conflicts now!

@thihara

thihara commented Oct 18, 2016

Copy link
Copy Markdown
Author

@dougwilson Any chance of approving this pull request soon?

@dougwilson

Copy link
Copy Markdown
Contributor

Sorry, I got busy this weekend. Let me just sync up with that other PR real quick to get alignment on the name. I meant to do that Sunday, but did not get to it. I'll provide an update soon with what I find out.

@thihara

thihara commented Oct 18, 2016

Copy link
Copy Markdown
Author

@dougwilson Thanks.

@thihara

thihara commented Oct 20, 2016

Copy link
Copy Markdown
Author

@dougwilson Any luck with reconciling this with the other PR?

@dougwilson

Copy link
Copy Markdown
Contributor

Not yet, but it has not been any time at all, in a place where many people (like myself) do this on our own spare time. For example, if someone asked a question in my repo while I was on a nice one week vacation, expecting me to respond instantly is not very fair. You're always welcome to comment on the linked PR as well.

@thihara

thihara commented Oct 20, 2016

Copy link
Copy Markdown
Author

@dougwilson Understood. I'm kind of under a looming deadline so I published a new npm module to bypass this until a resolution is met. But like I said, I'm not optimistic about the response time in the node-mime repository. :-\

@dougwilson

Copy link
Copy Markdown
Contributor

Sure, but there hasn't even been enough time to know. I have spoken with the author various time, and we even worked together to form the mime-db module this depends on. I don't think it's fair to assume that there will be no response.

If you are "under a deadline", there are a few things you can do now:

  1. Just add your repo to your package.json directly.
  2. Since we already export (and document) the .extensions map, the only thing you even need to do is clean up the MIME type string, if you even need to. A very simple way to do this PR user-land:
var mime = require('mime-types')

module.exports = Object.create(mime)
module.exports.allExtensions = function (str) {
  var type = /^\s*([^;\s]*)(?:;|\s|$)/.exec(str)
  return type && type[1] && mime.extensions[type[1].toLowerCase()] || false
}

Drop that into a file and require() it instead of mime-types directly.

@thihara

thihara commented Oct 20, 2016

Copy link
Copy Markdown
Author

@dougwilson My observation stemmed from the duration the pull request you referred to me has been open. But if you have personal experience, then that's great, hopefully you will get a response soon. Thanks, I've already published the new module and used it. I'll get rid of it when this is resolved. 👍

@dougwilson

dougwilson commented Oct 20, 2016

Copy link
Copy Markdown
Contributor

I've already published the new module and used it. I'll get rid of it when this is resolved.

Ah, didn't even realize :) It's not possible to remove modules from npm any longer, since the left-pad incident, afaik.

@thihara

thihara commented Oct 20, 2016

Copy link
Copy Markdown
Author

@dougwilson Really? Oh I didn't realize, I thought npm unpublish --force would do the trick.

@dougwilson

Copy link
Copy Markdown
Contributor

Ah, guess I didn't remember right. According to http://blog.npmjs.org/post/141905368000/changes-to-npms-unpublish-policy you can unpublish within 24 hours via command line, otherwise contact support as long as nothing is depending on the module or whatever :)

@dougwilson

Copy link
Copy Markdown
Contributor

Anyway, that's not to say publishing is bad, haha, I guess we're getting off topic :)

@dougwilson dougwilson added this to the 2.0 milestone Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants