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 to express 4.0 #145

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Update to express 4.0 #145

wants to merge 4 commits into from

Conversation

Rockstar04
Copy link

While its not officially out yet, it was actually a pretty quick change to get it working.

@dkulchenko
Copy link

express 4.0 has been released: https://github.com/visionmedia/express/releases/tag/4.0.0. +1 on the change.

@Rockstar04
Copy link
Author

I just want to add caution to my PR, I am relying only on the unit tests to claim that this works. I have very little experience with Locomotive so before anyone goes using this or merging it I would take a good look for yourself.

@RohovDmytro
Copy link

+1

2 similar comments
@sbiaudet
Copy link

+1

@th0r
Copy link

th0r commented Apr 28, 2014

+1

@just-boris
Copy link

express 4 already released, that's mean you can safely merge it

@@ -2,7 +2,7 @@
* Module dependencies.
*/
var router = require('actionrouter')
, mime = require('express').mime
, mime = require('send').mime

Choose a reason for hiding this comment

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

why are you using mime through send module instead of including mime directly?

Require mime directly instead of getting it through send
@Rockstar04
Copy link
Author

Thanks for finally looking at the changes @just-boris ! I'm a PHP developer, and all I knew is that it "worked" when I made those modifications.

I updated the dependencies, and the tests still passing so I assume is all well.

@Rockstar04
Copy link
Author

I believe the travis build result is an error with the test runner, testing for Node V10 and v8 both passed.

Just trying to reduce diff noise
Try to supress diff noise
@shawn-crossley
Copy link

It looks like travis does not support v4 anymore.

http://docs.travis-ci.com/user/languages/javascript-with-nodejs/#Provided-Node.js-Versions

+1

@Rockstar04
Copy link
Author

Thanks @shawn-crossley !

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.

7 participants