-
Notifications
You must be signed in to change notification settings - Fork 332
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
Incorrectly waits for stdin #136
Comments
I installed node v4.1.0 and ran What happens when you run this command?
Please provide any additional information you can about the problem. |
|
It looks like it's working. It may take a long time if your package.json is long since A user experience improvement would be to buffer the results to stdout so that you could see dependencies as they were found, instead of waiting till the end. This would require a significant refactor. Sorry I don't have a good answer. Let me know if you think of something that I haven't! |
I had pretty much the same issue last night, but it was apparently related to slow connection to npm registry. But as you said - it's not really intuitive at the moment and people can just stare there at blinking cursor and wondering that's going on. Is there any options for verbose output? |
It would be simple enough to print “Finding latest versions from npm…” at On Tue, Sep 22, 2015 at 5:46 PM Mr. Hyde notifications@github.com wrote:
|
Sounds good for me :) |
Note I had the same issue, was burning lot so CPU and not returning. Cleaned/removed node_modules directory fixed for me. |
@kalmanb Thanks... I'm not sure what is happening there, but definitely is problematic. Let's keep an eye on it, and if we can reproduce it with a given install state then we can debug. |
Refactoring so we can check each dependency and print it's state as we get it sounds good to me. It will also make it easier to support interactive mode (#95). Mean while, we could use the progress module or something similar, that allows us to show a progress bar in the CLI. The 'total' will be the number of dependencies and as we retrieve the latest version for each one, we will increase it by one. For example:
|
Good points. We spent a bit of timing discussing satisfied text, but it's probably worth changing in order to get incremental installs and interactive mode. Let's discuss in #95. Progress bar would be good! |
Are you using npm v3? It had pretty bad performance problems until recently (see http://blog.npmjs.org/post/130359991775/npm-weekly-31-npm-3-speed-fixes-modules-demoed and https://github.com/npm/npm/releases/tag/v3.3.6) which could explain the slowness |
@Daniel15 Nope, npm v2.14.4 + node v4.1.1. |
I'm tagging this |
I'm using npm v2.14.4 and node v4.1.1 with Windows 7, 64bit and ncu is not working. It starts, but never quits. Without any logs I cant check why. Tried to use it with "time ncu", because it may take longer, but I aborted it after 2 hours waiting. My package.json just has 16 entries. Any idea? |
Hi Michael, thanks for reporting. Clearly it is getting hung up somewhere. We should add verbose logs so that we could at least see where it is hanging. Until then, the best we could do is actually debug from the source code. Does it hang on an empty package.json? Adding console.log's or more robust logging is going to be the best approach to really find where in the code it hangs, but since I cannot reproduce you may be on your own for the time being. Perhaps someone else who can reproduce would be willing to debug. |
Yes. I tried to debug and added some logging to
Tested with an empty package.json returns:
And it never gets to analyzeProjectDependencies() and doesn't stop. |
Great information, thank you Michael. Maybe readFileAsync is not resolving? Can you tell if pkgData (promise) is resolving? Maybe add I wish I was able to reproduce and troubleshoot directly... |
@mriehema I added verbose logging to v2.40. Can you upgrade then report the output of the following command?
|
Awesome! I would upgrade and test it, if I could. :(
|
Snyk would be great if it worked out of the box, but it's not worth adding if it is going to break in some environments. I'll remove it. |
Try it now :). v2.4.1 |
@metaraine shouldn't synk be a dev-dep and not linked to install? npm install shouldn't do anything with synk |
@metaraine Thanks for your support. So I checked it:
BTW: I updated to node v4.2.2 and npm v2.14.7 So it looks like it still hangs up like I described. I added following code:
Well... Same output as before. Just no response of the Promise. |
Success! After some more debugging I tried to run it with explicit packageFile, and it works!
Looks like |
The question is why didn't it find the local package.json when you run it without parameters. "Finding local package file..." was supposed to succeed so it shouldn't have tried to read it from stdin. By the way, we can replace |
I have hit this issue on linux, when I have tried to invoke |
Any progress on this issue? Just curious... The provided workaround does work on Win 10 with Git Bash. |
@KingDarBoja I don't believe anyone is actively working on this. Open to pull requests though. |
@raineorshine Managed to solve the problem based on @kylios comment as I noticed the if-else check at Something like this: // ... More code
if (options.packageData) {
pkgFile = null;
pkgData = Promise.resolve(options.packageData);
} else if (options.packageFile) {
pkgFile = options.packageFile;
pkgData = getPackageDataFromFile(pkgFile, pkgFileName);
} else if (pkgFileName) { // extra check added :D
pkgFile = pkgFileName;
pkgData = getPackageDataFromFile(pkgFile, pkgFileName);
} else if (!process.stdin.isTTY) {
// ... More code
}
//... More code |
|
Personally I think it might make sense to make reading from stdin opt-in, as breaking change. Since arguably reading from stdin is the much less used branch. |
That makes sense to me. |
@raineorshine I know but as most users have to provide the package filename by using the @stoically makes sense if someone wish to use it, as a new flag which should take priority over the existing checks and everyone would (I think) be happy about it. |
No idea what changed on this library or my git bash but I no longer need |
@KingDarBoja Interesting. If an old version of |
I updated this week to |
Well that's good news at least. It's unclear whether it's node, npm, ncu, or one of ncu's dependencies that fixed it. If someone else can confirm I'll close the issue! |
hi,i meet with the problem:
and i use the code
i see this issue,so which is the answer to this problem? |
@hizablet Specify |
Just in case anyone ends up here while trying to run execa('ncu', { preferLocal: true, stdin: process.stdin }) For some reason it does not work without it and only when piping stdout are you able to get a warning message which links here. |
(As an aside, you can require npm-check-updates which may work better in some circumstances if you're willing to have it as a local dependency.) |
For anyone using An example: npx concurrently \
-n root,common,ui,api \
"npx npm-check-updates --packageFile package.json" \
"npx npm-check-updates --packageFile package.json --prefix common" \
"npx npm-check-updates --packageFile package.json --prefix ui" \
"npx npm-check-updates --packageFile package.json --prefix api" |
Have you solved the problem yet? I have meet the same problem... |
Auto stdin has been deprecated and will be removed in the next major version, which should fix the issue. In the mean time, does using |
Auto stdin removed in 7005bc6 |
Looks like ncu is currently not compatible with Node v4.1.0. I can run ncu, but nothing happens - cursor just goes to new line.
The text was updated successfully, but these errors were encountered: