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

Mimetype #143

Merged
merged 33 commits into from May 22, 2015
Merged

Mimetype #143

merged 33 commits into from May 22, 2015

Conversation

dotnetCarpenter
Copy link
Collaborator

Look story short: I really like The Mother of All Demos and looked into the modern version of HyperScope. Sadly, it is already based on ancient tech, so wanted to bring it up to date before it is too difficult. First choice was to use http-server. Unfortunately the .opml files HyperScope uses didn't load because of the mime-type/content-type and http-server has no way of defining custom types.

So I gave it try, fixing #66 and everything looks good but a few things, that I hope you guys can help with.

  1. Firstly I'm on windows right now, so get a lot of line endings equality errors (\r\n instead of \n) when I run the test suite. I think everything passes but it is difficult to see.
  2. I'm not sure how to write out warnings. In this case, when a path to a .types file is wrong (404). I would like to inform the user, not crash estatic or ignore the error.
  3. How do I go about testing number 2?

Anyway, I hope you look positive at this feature and perhaps I can reach my end goal 👍

@jfhbrook
Copy link
Owner

ecstatic doesn't have a way of emitting warnings or other log events, and I think that's out of the scope of this PR--though perhaps something I should consider! Given the current state and where you're doing the check, I would throw.

@dotnetCarpenter
Copy link
Collaborator Author

All right, I'll throw. Should I squash all the commits into 1? Or anything else?

@jfhbrook
Copy link
Owner

Ehh, I can manage squashing if I think it's necessary. In general I don't get overly finnicky about commit history of outside contributions because it would take way too much time.

@dotnetCarpenter
Copy link
Collaborator Author

Cool. So I'm almost there. Got an issue with the content-type.js test case. My teardown function doesn't properly tear down ecstatic. If you comment out line 63: 'mime-types': 'custom_mime_type.types' the test still passes because the mime-type for .opml is defined in the previous test. Any quick ideas to what would kill the ecstatic instance?

@jfhbrook
Copy link
Owner

You'll see that in a bunch of the tests I have timeout code that kills ecstatic if all the requests are done but a mystery open handle is still kicking around. I don't remember why I thought this was okay, but if it's good enough for the other tests...

@dotnetCarpenter
Copy link
Collaborator Author

I've asked for a reset implementation in mime of mime-types. broofa/mime#124

I'll try to spawn ecstatic in child process and then kill that between tests.

@dotnetCarpenter
Copy link
Collaborator Author

Hmm.. since I can't setup forks without support in ecstatic.js, which I rather not do just for testing and that the code we are testing passes each individual test in content-type.js, I'm inclined to let it go for now.
Interestingly, it seem that node cluster could be used. But t's out of scope for this PR.

@dotnetCarpenter
Copy link
Collaborator Author

@jfhbrook if I use setTimeout process.exit(0), then I would have to split content-type.js into 3 different files, right?

@jfhbrook
Copy link
Owner

I don't think so? You should be able to handle the timeout at the same place as you call server.close.

As an aside though, I don't think we have to test the mime package's behavior. We should assume it behaves as-advertised.

@jfhbrook
Copy link
Owner

I appreciate all the work you're putting into this by the way. Looks like good work!

@dotnetCarpenter
Copy link
Collaborator Author

I think it is good practice to test the parts of a third-party package that you use in your project. This automates detection of changes made in that library. It should make it very comforting to upgrade the package. If anything changes, we will know before pushing a new version with updated dependencies. I understand your stand point though. But I needed to learn mime anyway, so might as well do that in a unit test.

I'll give it a shot with timeout....

@dotnetCarpenter
Copy link
Collaborator Author

Nothing can happen after process.exit(0). So have to split up the test into 3 different files. In the long run, ecstatic should be able to proper clean up after itself. In long running processes or memory constraint devices, this could be a real issue.

@dotnetCarpenter
Copy link
Collaborator Author

I can't think of anything else to do in this PR. Ready for merge?

@dotnetCarpenter
Copy link
Collaborator Author

@jfhbrook wait. While implementing http-party/http-server#35 I realized that the path to the .types file should be independent of the root path. Say I want to serve files from one directory but want to load mime-types from another directory.

@jfhbrook
Copy link
Owner

Yeah, okay. I'll try to review some time this week.

@dotnetCarpenter
Copy link
Collaborator Author

@jfhbrook thanks. I know it's mouthful - it was for me - and will do what I can to help ease the merging of this. In the mean time, I'll continue work on http-party/http-server#35, based off this PR.

@jfhbrook
Copy link
Owner

You know ecstatic can be ran as a bin, right? npm install ecstatic -g && ecstatic

@dotnetCarpenter
Copy link
Collaborator Author

@jfhbrook eh, yes. What do you mean?

@jfhbrook
Copy link
Owner

I guess what I'm saying is, I don't understand what value http-server adds.

@jfhbrook
Copy link
Owner

ehh nvm, it looks like http-server ships with cors support and a few other things.

@dotnetCarpenter
Copy link
Collaborator Author

ah. I like http-server it super easy to use and I only knew about http-server, not ecstatic. I came to ecstatic because I wanted custom mimetypes. And since http-server depend on ecstatic... well, the rest is history. - yep it started here :)

@@ -1,4 +1,4 @@
# Ecstatic [![build status](https://secure.travis-ci.org/jfhbrook/node-ecstatic.png)](http://travis-ci.org/jfhbrook/node-ecstatic)
# Ecstatic [![build status](https://secure.travis-ci.org/jfhbrook/node-ecstatic.png)](http://travis-ci.org/jfhbrook/node-ecstatic) [![dependencies status](https://david-dm.org/jfhbrook/node-ecstatic.svg)](https://david-dm.org/jfhbrook/node-ecstatic)
Copy link
Owner

Choose a reason for hiding this comment

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

What is this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A huge issue I had, was updating from tap 0.4 to 1.0. Since I spend a few hours doing that, I though a badge was in order :). It's a free service that tells us about the health of our dependencies. See here: https://david-dm.org/jfhbrook/node-ecstatic#info=devDependencies&view=table (and don't worry - it will look better when you merge this PR)

@dotnetCarpenter
Copy link
Collaborator Author

@jfhbrook should mention that union-multiple-folders.js creates a [object global] folder in the root. I haven't checked why but pretty sure that that didn't happen before tap@1.0

@dotnetCarpenter
Copy link
Collaborator Author

hmm trying to figure out what @yfr wanted to do in union-multiple-folders.js. I'm not sure that test ever made sense. Did I delete something?

Add new or override one or more mime-types. This affects the HTTP Content-Type header.
Can either be a path to a [`.types`](http://svn.apache.org/repos/asf/httpd/httpd/trunk/docs/conf/mime.types) file or an object hash of type(s).

ecstatic({ mimeType: { 'mime-type': ['file_extension', 'file_extension'] } })
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jfhbrook, does this kind of doc make sense? An example would be ecstatic({ mimeType: { 'node/ecstatic+fun': ['ecs', 'fun'] } }) to get content-type: node/ecstatic+fun; charset=UTF-8 with .esc and .fun files.

Copy link
Owner

Choose a reason for hiding this comment

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

yeah, I think that makes sense.

@dotnetCarpenter
Copy link
Collaborator Author

@jfhbrook thanks for your time. Is there anything I can do to help this PR getting merged? I'm off-line this weekend but will be back mid next week.

@jfhbrook jfhbrook merged commit 4356c6e into jfhbrook:master May 22, 2015
@jfhbrook
Copy link
Owner

@dotnetCarpenter
Copy link
Collaborator Author

@jfhbrook that's awesome - thanks!

minor detail: though public is a fixture folder 8b27bba

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants