store types as json, compatible with browserify

[dominictarr]

This pull request makes mime bundle-able with browserify.

In particular this is useful, because mime is a dependency of request,
which can be used in the browser via browserify, but currently only with an old version of request (before 2.0.0)

All I have done is move Mime#load out into a build script,
than you can run after updating the types files.

node build.js types/* > types.json

OR

npm run build

That combines both type files into one types.json which is then loaded with require,
and then passed directly to Mime#define

cheers

[broofa]

Killing Mime.prototype.load breaks mime.load(), which I suspect quite a few people may be using.

Could this be done as a separate module that depends on node-mime?

I'm not familiar with how browserify works or how this fits into that world. Thus, I guess I need to be sold on the value of this change. If this is only solving a problem that you and one or two other people have... well... not really sure it's worth the added complexity.

[dominictarr]

Oh, sorry, I shouldn't have removed that! that was a breaking change.

I've put that back in.

browserify is extremely handy. it allows you to write modules in the node style, but use them in the browser,
so you can use npm to manage your dependencies. if you check the npm page https://npmjs.org/package/browserify

It has 144 dependant modules, which puts it in the top 100 modules, by dependants.

The thing that breaks this in browserify is these two lines:

https://github.com/broofa/node-mime/blob/master/mime.js#L90-L93

Which go on to read a file https://github.com/broofa/node-mime/blob/master/mime.js#L54

But, browserify can't tell what file that is, because it has to parse the js and look for dependencies,
and here it just sees a variable name. There is an extension for browserify that can handle readFileSync.
but it only works when you call readFileSync('./literal.json') with a literal file, and not a variable.

I could make this a module that depends on mime, but I can only if I could require mime without hitting
https://github.com/broofa/node-mime/blob/master/mime.js#L90-L93

The same problem would pretty much apply to use with other browser-bundlers.
Bundling also has other applications.
I am using browserify to create bundles that I can run inside the vm module,
safely sandboxed from the outside world.

[broofa]

How does browserify handle other modules that readFile()? That's gotta be a pretty common problem... so I assume there's a best-practice there.

[dominictarr]

modules that access the file system are generally server only,
for situations where you are loading unknown files at run time at least,
If you are only loading configuration, and the file is known before run time,
then that can be ported into the browser.

there have been many discussions about the best way to do this, to load non-js files into a bundle,
(say, html is a common target), and the best two approaches is either use brfs to detect readFileSync,
or to convert into json and then just use require (which can also load json!)

[dominictarr]

are you willing to merge this?
what about if I used a different approach that didn't use a json step?

It would moving a parse method out of load and calling it directly with the contents of the file.
so that readFileSync is passed a literal. Would that be acceptable?

[broofa]

Hey @dominictarr - sorry for the long delay on this. 'been on a bit of a node-mime hiatus. I need to think about this some more, but just wanted to let you know that I haven't completely disappeared. Feel free to aggressively harass me about this.

[dominictarr]

It would be awesome if this was merged - there is no reason that this can't be used in the browser, more portable is nicer than less portable, and this fix is only a small change. I think I've made a sufficient case for the merge, provided there is no additional arguments against it.

In the place where I needed this, I just used a git url in the package json to use my fork, but that is only a stopgap measure, and I'd be happy to see this merged.

But I'll leave that up to you.

[DamonOehlman]

👍 for merging

[puzrin]

Text files are used by others people for maintenance. It's very bad idea to convent those into unmaintainable format. Everything can be done transparently, when you build browserified version.

We have very similar mimoza project. It builds browserified version without adding ass pain to maintainers. Try it, o port it's browserification code here. If you wish to port, here is short instruction:

• move "rules loader" into separate file
• create shadow file for browser, and instruct browserify to substitute
• on browserification, convert text files into json & use them via require() instead of fs.

More details in sources & Makefile.

Regards!

[ianwremmel]

Any chance of this getting merged? It really would be great to be able to browserify request.

[ghost]

Another way to achieve the same effect of browserify support would be to add:

"browserify": { "transform": [ "brfs" ] }

to the package.json and "brfs" to the package dependencies. Then brfs will take care of inlining the file contents from fs.readFileSync() automatically.

[dominictarr]

@substack that doesn't work in this case, because the filenames are not passed to
the fs.readFileSync but to a load function, which calls fs.readFileSync, so it can't be statically parsed!

https://github.com/broofa/node-mime/blob/master/mime.js#L100-L105

[Fishrock123]

Given that this has been open for more than a year, we've recently made an alternative: https://github.com/expressjs/mime-types

Stores in JSON & etc. Can use the same api syntax as node-mime.

[rhalff]

I've made a new pull request which should fix this issue #96

I couldn't find a way to just attach the request to this thread.

[yanickrochon]

This PR should be updated. I came here just to see who'd propose such changes.

Having the mime types declared as a .json, using a simple require (which is synchronized) should also allow adding more information per mime types... for example the default charset, a common name, etc.

[Edit]

Instead of having

  "application/javascript": [
    "js"
  ],

why not

  "application/javascript": {
    "extensions": [ "js" ],
    "charset": "UTF-8"
  },

[broofa]

@yanickrochon : the main reason for not supporting charset info (or other 'meta' information around mimetypes) is that there isn't a canonical source for such mappings. At least not that I've seen, but I could be wrong.
... anyhow, because of that, adding charset mapping (beyond the primitive support already in place) adds complexity and maintenance burden that I'm not sure is appropriate for this library, but I'm happy to hear arguments to the contrary.

[dougwilson]

@yanickrochon we maintain these mappings in the mime-db npm module (with a more friendly interface in the mime-types npm module if you need it.

[dominictarr]

this problem is now pretty much solved because request now uses mime-types instead of this module.
great job!

[broofa]

@dominictarr @Fishrock123 @substack: All, my apologies for dragging my heels on this. Without going into detail, I've had other priorities because "life happened". I'm sorry.

I've just posted #107 which should resolve this issue, and also make it easier to pull in changes from apache.org. Can you let me know what you think? Assuming there's no objections I'll merge and publish this ASAP.

[Fishrock123]

No worries, I'll be posing in #108 in regards to mime-types.

[broofa]

Fixed in fdc1f31