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

Suggestions for why parallelshell breaks node-notifier? #109

Closed
simensen opened this issue Mar 29, 2016 · 10 comments
Closed

Suggestions for why parallelshell breaks node-notifier? #109

simensen opened this issue Mar 29, 2016 · 10 comments
Assignees

Comments

@simensen
Copy link

I've been able to get multiple people on multiple operating systems (OSX and Windows) to reproduce some unexpected behavior with the following package:

{
  "dependencies": {
    "node-notifier": "^4.5.0",
    "parallelshell": "^2.0.0"
  },
  "scripts": {
    "works": "npm run notify",
    "broken": "parallelshell \"npm run notify\"",
    "notify": "notify -t \"Some Title\" -m \"Some Message\" || echo 'failed'"
  }
}

For some reason when notify is called behind something run with parallelshell it will just hang. I'd love to try and figure out how I can get this unstuck but I'm not sure if it is a problem with parallelshell or node-notifier. :( Any ideas?

I've also opened an issue on parallelshel here: darkguy2008/parallelshell#52

@mikaelbr
Copy link
Owner

Hi. Thanks for the detailed and thorough issue! It seems to be an issue with the node-notifier bin using process.stdin.on('readable', fn) to listen for when the stdin stream is ready to send information, but when using parallelshell it never becomes readable. This might be an issue in how parallelshell spawns the process and what streams it uses when spawning. I don't fully know the details of parallelshell.

Let's wait until we get a response in your other issue to see what the maintainer says about it, if that works for you?

@simensen
Copy link
Author

@mikaelbr works for me. :) however, i see that parallelshell is now marked as not maintained. :( A friend of mine (@silentworks) has been helping me through this and I'm going to try using another (similar) library to parallelshell to see if that unsticks me. :)

I figured it had something to do with spawning/exec/child process something-or-another but I'm not familiar enough with process-level stuff (at list with node) to even start tracking this theory down. Thanks for looking into it! If I find a solution using another library I'll update this ticket. It might be worth adding to a FAQ or on the README of notifier in case other people run into this. If you have a preference on that let me know and I'll see if I can contribute a note to the docs.

@mikaelbr
Copy link
Owner

Contribution is very much welcome. If you feel like helping out, you can add a section to the Common Issues.

@simensen
Copy link
Author

@mikaelbr ok, great. fwiw, while working on this problem yesterday i was able to find a workaround by replacing parallelshell with npm-run-all. however, at that point i found that it was still broken in the case that i was using nodemon in another part of my build process (for watching). I've updated my little sample to show all of the cases that work, don't work, or work in ways that are not super nice (watch works but is super slow at detecting filesystem changes compared to nodemon).

{
  "dependencies": {
    "node-notifier": "4.3.1",
    "nodemon": "^1.9.1",
    "npm-run-all": "^1.6.0",
    "parallelshell": "^2.0.0",
    "watch": "^0.17.1"
  },
  "scripts": {
    "notify": "notify -t \"Some Title\" -m \"Some Message\" || echo 'failed'",
    "works-simple": "npm run notify",
    "works-npm-run-all": "npm-run-all --parallel notify",
    "works-watch--but-slow": "watch 'npm run notify' .",
    "broken-parallelshell": "parallelshell 'npm run notify'",
    "broken-nodemon": "nodemon -q -w . --exec 'npm run notify'"
  }
}

My guess is that nodemon is doing something similar to parallelshell in how it spawns processes while npm-run-all and watch are doing something that is more compatible with notify.

I had hoped that watch would work but the delay is suboptimal and a downgrade from the previous speed/performance we were seeing with nodemon. The whole notification stuff was supposed to aid things and not slow stuff down so I think I'll just have to ditch notify for awhile.

If you find a fix for this and end up closing this issue I'll know I can come back and hopefully get going more quickly than this pass. :) I've spent a lot of time on this so far (more time than I'd like to admit) and think I need to move on quickly. :)

I think it makes sense to leave this issue open as an unconfirmed bug until such time as you confirm it (one way or another) or fix it (yay!) so I won't close it myself. If you decide to close it for now that is fine with me. :)

I'll still try to get something into common issues over the next day or two if I get some free time.

Thanks for your help and looking into it!

@mikaelbr
Copy link
Owner

I'll look into this some more. Maybe I'll find a better way to support pipe functionality that won't break this behaviour.

@simensen
Copy link
Author

simensen commented Jun 6, 2016

@mikaelbr Thanks for taking a crack at fixing this. I'm still not seeing it working, though. I updated my sample package.json to include 4.6.0 and the two broken scripts (broken-parallelshell and broken-nodemon) still fail even using 4.6.0.

{
  "dependencies": {
    "node-notifier": "4.6.0",
    "nodemon": "^1.9.1",
    "npm-run-all": "^1.6.0",
    "parallelshell": "^2.0.0",
    "watch": "^0.17.1"
  },
  "scripts": {
    "notify": "notify -t \"Some Title\" -m \"Some Message\" || echo 'failed'",
    "works-simple": "npm run notify",
    "works-npm-run-all": "npm-run-all --parallel notify",
    "works-watch--but-slow": "watch 'npm run notify' .",
    "broken-parallelshell": "parallelshell 'npm run notify'",
    "broken-nodemon": "nodemon -q -w . --exec 'npm run notify'"
  }
}

@mikaelbr
Copy link
Owner

mikaelbr commented Jun 6, 2016

Hm. That's weird. I got all of those examples working when developing and testing out. But there are some issues here now. I might have messed it up. Will look more into this as soon as I can. Sorry about that.

@mikaelbr mikaelbr reopened this Jun 6, 2016
@simensen
Copy link
Author

simensen commented Jun 6, 2016

@mikaelbr No rush. I do appreciate you taking the time to look into it at all. :) I'll try to test it again as soon as I see you've made a change but it took me almost two weeks last time.

@mikaelbr
Copy link
Owner

Closed in favour of mikaelbr/node-notifier-cli#1 as CLI has moved to own project.

@bonafideduck
Copy link

FWIW, I'm having a similar issue (my setup is a bit convoluted because it uses npm-watch which uses nodemon under the hood and I have a bunch of && and ||, but I think the workaround that worked for me was to add stdin:

{
  "scripts": {
    "fail": "echo | notify -t 'Warning' -m 'Build failed.'",
    ...
  }
  ...
}

Here is the entire scripts:

  "scripts": {
    "dev": "npm-watch",
    "run_dev": "npm run build && npm run chrome || npm run fail",
    "build": "tsc && vite build",
    "chrome": "open -a 'Google Chrome' http://reload.extensions",
    "fail": "echo | notify -t 'Warning' -m 'Build failed.' && false"
  },

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

No branches or pull requests

4 participants