-
-
Notifications
You must be signed in to change notification settings - Fork 190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update mime #151
Update mime #151
Conversation
Awesome, thanks! Before I can merge this, I need to make the following fixes (I'm listing them here in case you have some time to help with them, but otherwise I'll take care of them):
|
@dougwilson done! 👍 |
Thanks! I feel like there would have been more than ome change between 1.4.1 and 1.6.0 (i.e. there was a 1.5.0 at least)? I'll take a look into this unless you're saying there wasn't any other changes. Also I need to remove the version and date from the top of the HISTORY file before I can get it merged and remove the unrelated line changes. I'm not asking you to make these changes unless you want to, just being transparent in the changes I will be making to the PR before merging. |
@dougwilson I did the following changes:
Please let me know if any further changes are needed. Thanks for the help! |
Thanks @castilloandres ! I'm not sure that is right, because at least one of those (the vulnerability one) was already included in a prior version (1.4.1). I would expect to only see the changes between 1.4.1 and 1.6.0 included in the notes. I'll take a look at the |
@dougwilson indeed the fix was included from |
bump, can we get this in? |
Just need the history updated to reflect the actual changes included here. |
Also looks like there is an update to the history file for a very old version of this module. Not sure why that is being updated in the PR. Can someone explain? |
here's the diff: broofa/mime@v1.4.1...v1.6.0 |
Which one of those commits does the entry |
oh hmm, that commit doesn't actually look like it's in the diff... only in the 2.x series? broofa/mime@1df903f changelog lies |
That's an issue, not a commit that was included in that diff you provided. That issue was resolved in |
yup agreed, it shouldn't be in the history here. |
Like I said, the included change list in this PR is not correct. I can merge it when it's fixed up. Here are the issues I see blocking merging right now:
Once this is resolved, this will create a candidate for a new minor version of this module. Versions of this are locked with Express and so will need to schedule a new minor version of Express 4.x as well. Once the scheduling is done, we'll get new releases cut. We don't even start the scheduling process until there is at least one candidate change, and this PR is still pending updates. I hope that helps provide the state of the PR @devongovett |
I'm looking for correct mime type for I'll make a new PR and address the issues you raised. |
Thanks @devongovett . Please verify with @castilloandres that that is OK. Of course just as an FYI you can always define new MIME types and extensions with this module; you are not locked in to the ones included by default -- they are just the defaults not the only possible values. |
Closing this PR. Following up on #154 |
Update mime package from
v1.4.1
tov1.6.0
to fix -> expressjs/express#3486