search

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

Added EventEmitter related method types for supporting newer versions of Nodejs v14.x.x or higher #85

Closed KRTirtho closed 3 years ago

KRTirtho commented 3 years ago

Issue fix for #84

Changes

Tested Environments

Nodejs: v13.5.0 & v14 .15.4 (works in both) @types/node: v14.14.22

j-holub commented 3 years ago

Sorry for the late response, I wanted to take some time for this and I'm not that familiar with typescript types either.

I see quite some of these changes are formatting changes, is there any reason for that. When I look at both version locally, they look exactly the same, however VS Code also tells me that the lines have changes. I just don't see it. Can you maybe elaborate on that? Why the whole file looks as changed allthough you just added some lines? I don't really understand it, is it some tab vs spaces thing? And also, what's the reason for the change in package.json ?

Also, @leonekmi , would you mind taking a look at this PR as well, since you're more knowledgable about TypeScript. If you don't have any time or you don't want to I understand that though.

Thanks @KRTirtho for taking the time to make this PR, it's really appreciated.

KRTirtho commented 3 years ago

Thanks @j-holub. Sorry I was busy for 2 days...

Why the whole file looks as changed allthough you just added some lines? I don't really understand it, is it some tab vs spaces thing?

I think its a tab/space related issue actually I use spaces instead of tabs & also typescript's default formatter. Also sometimes this kind of thing happens for CRLF & LF line break schemes.. As I'm using Linux my system uses the LF line breaks in any file. So it is possible to happen if your machine is running Windows as windows uses CRLF for line breaking & git isn't converting the CRLF > LF which git does by default if it is running on a non-unix based system... Also I use VSCode's format on save so this might modified the whole file

And also, what's the reason for the change in package.json ?

I made that change for testing as I changed the path of index.js for testing purposes & then reverted it back to ./index.js which was in fact index.js.... This happened because of my forgetful nature, sorry!😅 But I fixed this on a next commit...😄 Same thing happened for index.d.ts

KRTirtho commented 3 years ago

@j-holub, its been a week now I created the PR... If you're free then please check this PR & inform me if there was any problem in the code... Thanks..

j-holub commented 3 years ago

Hey man,

super sorry about not getting back to you, it kinda just got lost in the daily life. Really sorry. I think we we're talking about the whitespaces, I don't really have a strong opinion about whitespaces vs tabs, I just would like them to be consistent throughout the project. Except for that the PR is great and I'd like to merge it : )

KRTirtho commented 3 years ago

There's no big deal about the tabs or spaces... I just use spaces as it takes less space & keeps the view within a perfect wrap in my opinion. So don't worry I'll be changing this back to default... Thanks @j-holub for taking out time even after being so busy.. I'll do it right way

KRTirtho commented 3 years ago

@j-holub to view the current changes with your repository use this diff link https://github.com/j-holub/Node-MPV/compare/master...KRTirtho:master... Hope this commit would fixed everything & it can be merged

Thanks...

j-holub commented 3 years ago

I'm the one who has to thank you, I really appreciate your help with this module.