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

lib: work around node.js bug #205

Merged
merged 1 commit into from
Sep 19, 2015
Merged

lib: work around node.js bug #205

merged 1 commit into from
Sep 19, 2015

Conversation

LinusU
Copy link
Member

@LinusU LinusU commented Aug 14, 2015

This should hopefully fix a number of bugs where multer/the browser appears to hang.

I'm waiting on @mscdex comment in nodejs/node#2325 before merging thought, I might be fucking up something important.

Test this fix by installing as such:

npm install expressjs/multer#d787546

@LinusU
Copy link
Member Author

LinusU commented Aug 14, 2015

So it seems like this breaks on Node.js 0.10 😭 I'm going to bed now, I'll take a look at it later.

@zyf0330
Copy link

zyf0330 commented Aug 17, 2015

npm WARN `git config --get remote.origin.url` returned wrong result (git://github.com/expressjs/multer) undefined
npm WARN `git config --get remote.origin.url` returned wrong result (git://github.com/expressjs/multer) undefined
npm WARN `git config --get remote.origin.url` returned wrong result (git@github.com:expressjs/multer) undefined
npm WARN `git config --get remote.origin.url` returned wrong result (git@github.com:expressjs/multer) undefined
npm ERR! git clone git@github.com:expressjs/multer undefined
npm ERR! git clone git@github.com:expressjs/multer undefined
npm ERR! Windows_NT 6.3.9600
npm ERR! argv "D:\\Program Files\\nodejs\\\\node.exe" "D:\\Program Files\\nodejs\\node_modules\\npm\\bin\\npm-cli.js" "install" "expressjs/multer#d787546"
npm ERR! node v0.11.14
npm ERR! npm  v2.0.0
npm ERR! code ENOGIT

npm ERR! not found: git
npm ERR!
npm ERR! Failed using git.
npm ERR! This is most likely not a problem with npm itself.
npm ERR! Please check if you have git installed and in your PATH.

npm ERR! Please include the following file with any support request:
npm ERR!     D:\project\Node\caitian_node\npm-debug.log

this is what was shown when I run 'npm install expressjs/multer#d787546',I don't know why.
When I run 'npm install multer', it is normal.

@LinusU
Copy link
Member Author

LinusU commented Aug 17, 2015

It seems like you don't have git installed.

If you don't want to install it you could probably download this version manually (link below), unzip it, and install it with npm install C:\path\to\folder.

https://github.com/expressjs/multer/archive/d787546fdb389d8690cbef9a4103a0e5d4025294.zip


edit It's probably easier to just do

npm install https://github.com/expressjs/multer/tarball/d787546fdb389d8690cbef9a4103a0e5d4025294

@zyf0330
Copy link

zyf0330 commented Aug 17, 2015

@LinusU It's OK!!!
you can put it into master now.

@LinusU
Copy link
Member Author

LinusU commented Aug 17, 2015

I think that we need to figure out why it doesn't work on 0.10.x first... Or just drop support for it, but that would require a new major version.

@zyf0330
Copy link

zyf0330 commented Aug 17, 2015

I just see that it only occurs on NodeJS 0.10.x.
The reason why I use 0.11.14 is that node on version after it will cause liveedit of debug of Webstorm working error. Oh, I use 0.11.14 not 0.10.x.

I think maybe I can't give you some advice useful,because I don't know these much.

@LinusU
Copy link
Member Author

LinusU commented Aug 17, 2015

The problem is that this fix only works on 0.11 and newer, it doesn't fix the problem on 0.10. This can be seen in the travis build.

@zyf0330
Copy link

zyf0330 commented Aug 17, 2015

Oh sorry,I know now.I can only wish you to solve it early.

@LinusU
Copy link
Member Author

LinusU commented Sep 19, 2015

Rebasing to get Travis to run on more versions of Node

@LinusU
Copy link
Member Author

LinusU commented Sep 19, 2015

ping @jpfluger @hacksparrow

I'm merging this now since we are getting a ton of bug reports because of this problem. My reasoning for it being a minor version bump is because I'm not breaking anything that worked before. I'm merely fixing it for all supported versions of Node, expect for 0.10.

A lot of tests that was previously working are now broken not because of my changes in the library, but because I updated the tests to check that the stream is fully drained before considering the request as done (since this is what web browsers do).

Node 0.10 was released 2013-03-11, that's a long time ago, especially in node land. The current stable version is 4.1.0 which is miles ahead. I don't see a reason why we should support 0.10.

I propose that we drop support for 0.10 at the next major version (2.0) unless someone finds a workaround for the Node.js bug that works for 0.10 as well. Until then, the tests will be allowed to fail on Node.js 0.10 as they are failing because of an error in Node.js, not in multer.

I hope that this sounds good to you guys, let me know otherwise and we'll work something out. Cheers!


So I actually managed to fix support for 0.10 and dropping the workaround specific code!

Celebrate!

Lets get this party started! I'm cutting a new release now 💯👍

LinusU added a commit that referenced this pull request Sep 19, 2015
@LinusU LinusU merged commit 039c81e into master Sep 19, 2015
@LinusU LinusU deleted the nodejs-stream-bug branch September 19, 2015 22:13
@jpfluger
Copy link
Contributor

Great job @LinusU! Kudos to @mscdex and the node-group for fast turnaround/help. Sweet. 🎆

@hacksparrow
Copy link
Member

👍

@zyf0330
Copy link

zyf0330 commented Sep 20, 2015

I still use multer@1.0.3 and that problem doesn't occur now.Maybe the reason is I update NodeJS into the newest version v4.0.
@LinusU

@LinusU
Copy link
Member Author

LinusU commented Sep 20, 2015

@zyf0330 The patch hasn't been merged yet but maybe. I would still recommend updating to 1.0.5.

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.

4 participants