Skip to content
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

Closed
wants to merge 11 commits into from
Closed

Conversation

castilloandres
Copy link
Contributor

@castilloandres castilloandres commented Dec 1, 2017

Update mime package from v1.4.1 to v1.6.0 to fix -> expressjs/express#3486

@dougwilson dougwilson self-assigned this Dec 2, 2017
@dougwilson
Copy link
Contributor

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):

  1. Remove the version bump from package.json
  2. Remove the package-lock.json file
  3. Add an entry to HISTORY.md describing that the dependency has changed and what the changes are

@castilloandres
Copy link
Contributor Author

@dougwilson done! 👍

@dougwilson
Copy link
Contributor

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.

@castilloandres
Copy link
Contributor Author

@dougwilson I did the following changes:

  1. Removed date and version in HISTORY.md
  2. Added missing changes from node-mime @1.5.0 in HISTORY.md
  3. Put trailing spaces back in HISTORY.md. My sublime removes them when saving the file.

Please let me know if any further changes are needed.

Thanks for the help!

@dougwilson
Copy link
Contributor

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 mime library to see what the changes between those versions are.

@castilloandres
Copy link
Contributor Author

@dougwilson indeed the fix was included from mime@1.x so I went ahead and updated the HISTORY.md.

@devongovett
Copy link

bump, can we get this in?

@dougwilson
Copy link
Contributor

Just need the history updated to reflect the actual changes included here.

@dougwilson
Copy link
Contributor

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?

@devongovett
Copy link

here's the diff: broofa/mime@v1.4.1...v1.6.0

@dougwilson
Copy link
Contributor

Which one of those commits does the entry Fix vulnerable regular expression refer to?

@devongovett
Copy link

broofa/mime#167

@devongovett
Copy link

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

@dougwilson
Copy link
Contributor

That's an issue, not a commit that was included in that diff you provided. That issue was resolved in mime version 1.4.1, though. Why would that be listed in the changes here?

@devongovett
Copy link

yup agreed, it shouldn't be in the history here.

@dougwilson
Copy link
Contributor

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:

  1. The change log doesn't actually describe the changes of mime 1.4.1 to 1.6.0
  2. There is no description of what types are being changes here that would help anyone understand what is happening. For example @devongovett I assume you're looking for some particular change. Is that listed in the history file here?
  3. There is a change included lower down in the HISTORY file for an older version. Not sure why that change is there. Should it actually be at the top with the rest of the changes?

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

@devongovett
Copy link

I'm looking for correct mime type for .wasm files for the Parcel dev server: https://github.com/parcel-bundler/parcel. We just use serve-static directly without express, so once that and send are updated, we'll be set.

I'll make a new PR and address the issues you raised.

@dougwilson
Copy link
Contributor

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.

This was referenced Jan 5, 2018
@devongovett
Copy link

#154

@castilloandres
Copy link
Contributor Author

Closing this PR. Following up on #154

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.

None yet

3 participants