j-holub / Node-MPV

A NodeJs Module for MPV Player
https://www.npmjs.com/package/node-mpv
MIT License
116 stars 73 forks source link

fixes 2 issues that prevented start() from resolving. #72

Closed twilson90 closed 4 years ago

twilson90 commented 4 years ago
j-holub commented 4 years ago

Thanks a lot for the contribution :)

I am still discussing race conditions with the guy from mpv, apparently waiting for idle or file-loaded isn't super safe, because the events might fire before the node socket that listens for them is connected.

I still don't know why you would want to pass files via command line in the part where you create the mpv object, but I also don't want to shove my habits down other peoples throats.

However, this got me thinking that I should add command line options to the start() method, maybe even move them completely from the constructor to the start method. Back in version 1 there was no start method and the costructor already started mpv, so it's kind of some legacy behaviour.

twilson90 commented 4 years ago

I've run into some more issues with the new mpv start check promise... If you try to pipe in a stream it doesn't work.

I think it might be best to revert to just checking for the idle event and informing users in the readme never to add input files as mpv arguments.

j-holub commented 4 years ago

What kind of problems? And what would you want to revert? The part with stderr or the promise that listens to file-loaded?

To be honest, I don't know why you would want to pass files as a command line argument anyways...

twilson90 commented 4 years ago

I'm referring only to the bit where I added 'file-loaded'. That should be removed and we should discourage (or possibly disallow) users from inputting files as spawn arguments.