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

Switch to mime-types from mime #192

Merged
merged 1 commit into from
Feb 3, 2022
Merged

Conversation

LinusU
Copy link
Contributor

@LinusU LinusU commented Mar 5, 2020

This was picked up from the Express 5.0 todo-list: expressjs/express#2237

Use mime-types instead of mime

accepts is already using mime-type so currently both are being pulled in to Express:

$ npm ls mime-types
express@5.0.0-alpha.7 /Users/linus/coding/express
├─┬ accepts@1.3.7
│ └── mime-types@2.1.26 
├─┬ supertest@3.3.0
│ └─┬ superagent@3.8.3
│   └─┬ form-data@2.5.1
│     └── mime-types@2.1.26  deduped
└─┬ type-is@1.6.18
  └── mime-types@2.1.26  deduped

$ npm ls mime      
express@5.0.0-alpha.7 /Users/linus/coding/express
├─┬ send@0.16.2
│ └── mime@1.4.1 
└─┬ supertest@3.3.0
  └─┬ superagent@3.8.3
    └── mime@1.4.1  deduped

Note that supertest is a dev-dependency so after this change only mime-types should be present when adding express as a dependency.


Proposal for changelog:

0.18.0 / 2020-03-XX
===================

  * **Breaking:** Switch to `mime-types` from `mime`

    Migration guide:

    The mime type database is no longer extendable at runtime, and thus the `.mime` export have been removed.

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

Since this is a breaking change can this target the 1.x branch along with the other breaking changes for 1.x?

Also, we need to at least document some method for users to change the mime types for files based on extension... that would be a very large and strange API gap to introduce, as setting that type of thing is common among all static file servers.

Please include the updates to the history file as part of the PR itself :)

@LinusU
Copy link
Contributor Author

LinusU commented Mar 5, 2020

Since this is a breaking change can this target the 1.x branch along with the other breaking changes for 1.x?

Sure thing ✔️

Also, we need to at least document some method for users to change the mime types for files based on extension... that would be a very large and strange API gap to introduce, as setting that type of thing is common among all static file servers.

Sounds good! I'm guessing that we should introduce an API specifically for this? I'll post a suggestion on it soon ✔️

Please include the updates to the history file as part of the PR itself :)

Will do ✔️

@dougwilson
Copy link
Contributor

Sounds good! I'm guessing that we should introduce an API specifically for this? I'll post a suggestion on it soon ✔️

We may need to, yes. Currently mime-types itself doesn't have one, but we probably need to do something as it is quite the feature gap. The mime module was archived for a while but has been since unarchived. The motivation behind this change is partly due to that and to simplify the dep tree.

I think we may also decide there is too much work to do still to throw it into express 5 as well, as I think we all agree a top goal is to prevent further slipping of the release, lol.

Sorry I don't have a direct opinion right now; just quickly typing at work and will give more thought, especially if you have more to post before I'm out tonight.

@LinusU
Copy link
Contributor Author

LinusU commented Mar 5, 2020

I think we may also decide there is too much work to do still to throw it into express 5 as well, as I think we all agree a top goal is to prevent further slipping of the release, lol.

100%, I don't want this to hold up the release at all 👍

Sorry I don't have a direct opinion right now

No worries!


Looking at the current API, I'm not a fan of extending global variables. Especially since any library can import mime and add something to it, but that might not always be the same mime that send will use depending on how the dependency tree looks. I think it lends itself to produce bugs that are hard to catch...

One idea for an API would be something like this:

  • In send we add an option to pass in a mime-type database/source object. In a breaking change, we could default this to require('mime-db').
  • In serve-static we add the same option, which would just be passed to send. In a breaking change, we could bump the send version so that it defaults to mime-db.
  • In Express we add the same option as a global .set() option that, if set, gets passed to serve-static/send. In a breaking change, we could bump the serve-static/send version so that it defaults to mime-db.

On top of this we would probably also need an extend function that can extend an existing mime database object, so that people can easily add there own mime types.

The usage would look something like this:

Send

const send = require('send')
const mimeDB = require('mime-db')

const mimeTypes = { ...mimeDB, 'application/linusu': { ext: ['.foo'] } }

// ...

send(req, 'linusu.foo', { mimeTypes })

serve-static

const serveStatic = require('serve-static')
const mimeDB = require('mime-db')

const mimeTypes = { ...mimeDB, 'application/linusu': { ext: ['.foo'] } }
const serve = serveStatic('public/linusu', { mimeTypes })

// ...

serve(req, res, (err) => console.error(err.stack))

express

const express = require('express')
const mimeDB = require('mime-db')

const app = express()
const mimeTypes = { ...mimeDB, 'application/linusu': { ext: ['.foo'] } }

app.set('mime types', mimeTypes)

app.get('/', (req, res) => {
  res.sendFile('linusu.foo')
})

app.use(express.static('public/linusu'))

// ...

The beauty with this approach:

  • We don't have to make it a breaking change
    • (the only breaking change would be defaulting to the new mime-db)
  • No global variables, but still easy to configure your entire Express project
  • Users can use alternative mime databases
    • (e.g. one with only the types that they are going to use)

What do you think a good approach to move this forward is? Open an issue on expressjs/discussions to start a discussion on the API design? or submit draft PRs implementing my suggestion? or something else? ☺️

@dougwilson dougwilson mentioned this pull request Feb 3, 2022
6 tasks
@dougwilson dougwilson changed the base branch from master to 1.0 February 3, 2022 01:55
dougwilson pushed a commit to LinusU/send that referenced this pull request Feb 3, 2022
Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

I know this issue is old, and kind of fell off the radar. I'm going to last this as-is on the 1.0 release branch here, but you make a lot of good points in your last issue. I figure we could probably get that implemented prior to 1.0, or even after, it doesn't really matter, since it should be semver minor.

@dougwilson dougwilson merged commit af94f7b into pillarjs:1.0 Feb 3, 2022
dougwilson pushed a commit that referenced this pull request Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants