No mpv.on property in v2.0.0@beta.1

[KRTirtho]

Bug Description

Calling mpv.on("status") throws TypeError property on doesn't exist on mpv even checking if the mpv.isRunning()

How To Reproduce

//working with react-nodegui
useEffect(()=>{
(async ()=>{
    await mpv.start();
    await mpv.load("path/to/file");
})()
if(mpv.isRunning()){
      mpv.on("status", (status)=>{
        console.log(status) //throws TypeError
      })
   }
}, [])

Expected behavior

This should log the status when any status is changed of the mpv player

Software Versions

• Node-Mpv: v2.0.0@beta.1
• MPV: v6.14.11
• OS: KDE Neon Linux running Ubuntu LTS 20.04 base. Linux kernel - 5.4.0-64-generic

I believe its a typescript related issue as the types of on events aren't defined in the index.d.ts. I'm not familiar with this package that much so I didn't go deeper. Thanks in advance for making this awesome node_module & for helping regrading the issue.

[j-holub]

Hey there,

sorry for the late response, didn't see it. That's weird, I never had that one before, when you're saying TypeScript error, does this happen at compile time or runtime? I haven't written the types of this package, I'm not too familiar with TypeScript either, I believe it was @leonekmi who contributed the types. Did you experience anything similar?

I haven't tried to reproduce the issue yet, but I will and come back to you.

[KRTirtho]

Yeah, it was thrown by npm run dev when webpack recompiled my edited code. I believe its a typescript issue. Also there are no .on or .off handlers in the described types.. That's why I assume its a typescript related issue.. Another weird thing happened that if use it like (mpv as any).on(...) that still throws same error. Which shouldn't be happening if it was only a typescript related issue. Thanks for response

[leonekmi]

TypeScript is never to blame when you face a runtime error, especially if your code is plain js. TypeScript does only typechecking while building TypeScript code, never in runtime, TypeError is a pure JavaScript error. Are you creating the mpv instance using the new keyword?

The NodeMpv class in type declaration is extending the EventEmitter interface (like the real code does), making on and off available for TypeScript users in theory. Try installing @types/node if you haven't already, it may fixes your issue in TypeScript not recognizing on and off functions.

[KRTirtho]

Well I found the issue. The problem is NodeJS.EventEmitter is a interface not an abstract or a class. So it can't be extended but can be implemented. @leonekmi what wrong you did is that you extended NodeJS.EventEmitter instead of implementing

Checkout @types/node's event.d.ts file https://github.com/DefinitelyTyped/DefinitelyTyped/blob/a17292911589e315b72ca8034cb9c8ac5eff4030/types/node/events.d.ts#L56

Also checkout index.d.ts of this repo https://github.com/j-holub/Node-MPV/blob/86bc0cb302d04fcef6e2877995618840077134bd/index.d.ts#L35

Thanks for helping me this far...Hope you understand what I said

versions

Node: 14.15.4 @types/node: 14.14.22

[j-holub]

Hey guys,

first of all, thanks @leonekmi for taking the time to reply to this thread and help out.

@KRTirtho , so this is a general issue impacting all users of Node-MPV with TypeScript, right? Would you mind providing a pull request for the fix? I'll merge it right away.

[KRTirtho]

Of course I'll. So I've to get deeper in this now. Give me 2 days I'll try to fix this issue ASAP & will make a PR. It would be helpful if you provide me with the emitted event names & properties/data or reference to them of this node_module. This would save a lot of time. I know only three of 'em status, stopped & maybe progress...

Thanks @j-holub for making this great library & helping me out from this issue

[j-holub]

Take all the time you need, this is an open-source project we work on in our free time : )

It's been a while that I developed the basis of the module but the possible event strings are exactly those mentioned in the readme.md under Events There's a file called events.js where messages received from mpv are handled and the according events are emitted.

Let me know if you need anything else or some kind of assistance, we can also work on a shared branch together for a PR if you need some help.

Thank you for using it, it always makes me happy that this small project I started with an over-night session is still used by quite some people.

[j-holub]

How To Reproduce

//working with react-nodegui
useEffect(()=>{
(async ()=>{
    await mpv.start();
    await mpv.load("path/to/file");
})()
if(mpv.isRunning()){
      mpv.on("status", (status)=>{
        console.log(status) //throws TypeError
      })
   }
}, [])

One thing I just realized abut your code, since that first part is running asynchronously, it could happen, that the if part is running before mpv.start() has completed so I think you got yourself a little race-condition there. That might also cause problems down the road.

[KRTirtho]

Don't worry I already experienced some issue because of it. So I always mpv.quit() the previous session in a useEffect cleanup function for bypassing any memory leak or race condition.

Thanks for helping me for the event names & the suggestions.

[KRTirtho]

Sorry @leonekmi. It wasn't your fault but there was a breaking change after @types/node@13.6.x. As you created this typings before @types/nodejs@v14.x.x you had to use EventEmitter as class but after @types/node@v13.6.x it was converted into a interface & was moved to event.d.ts from global.d.ts. So now I'll create the new version based on EventEmitter interface

Thanks again

[j-holub]

Is there a way to make it possible that this module works both with NodeJs >=14 and NodeJs <14?

[KRTirtho]

As long as the user uses the latest @types/node@v13.7.x or higher package, it should work. And sorry I mistakenly said node but it should be @types/node. Also, yeah it works in versions of nodejs lower than v14.x.x.

I finished writing the types supporting the newer versions of node & will be making a PR soon. Thanks

[j-holub]

Fixed with #85 thanks to @KRTirtho