junlarsen / league-connect

:electric_plug: Node.js HTTP/1.1, HTTP/2.0 and WebSocket interface to the League of Legends Client APIs
https://www.npmjs.com/package/league-connect
MIT License
156 stars 24 forks source link

Replace depricated WMIC with Get-CimInstance #54

Closed Nerdsie closed 2 years ago

Nerdsie commented 3 years ago

WMIC is deprecated, and even removed on Windows 11 dev (not sure about beta/release)

junlarsen commented 3 years ago

Have you tested this locally yourself? I pulled it down myself, and none of the tests are passing out of the box on my machine.

E: My system complains that Get-CimInstance isn't a recognized cmdlet in path, which means it's not executing in PowerShell. Passing { shell: 'powershell' } as the second argument to exec solves this for users whose default shell is not PowerShell.

Nerdsie commented 3 years ago

My bad for not testing locally

Thank you so much for the patch (and the heads up about WMIC)! It's greatly appreciated.

I've tested it locally, and I'll be happy to accept the patch with the following changes:

diff --git src/authentication.ts src/authentication.ts
index 72d83d2..99ef778 100644
--- src/authentication.ts
+++ src/authentication.ts
@@ -110,7 +110,7 @@ export async function authenticate(options?: AuthenticationOptions): Promise<Cre
         : `ps x -o args | grep '${name}'`

     try {
-      const { stdout } = await exec(command)
+      const { stdout } = await exec(command, { shell: 'powershell' })
       const [, port] = stdout.match(portRegex)!
       const [, password] = stdout.match(passwordRegex)!
       const [, pid] = stdout.match(pidRegex)!

Is this going to cause issues with non windows systems? Maybe a better option would be something like

const args =
      process.platform === 'win32'
        ? [`Get-CimInstance -Query "SELECT * from Win32_Process WHERE name LIKE '${name}.exe'" | Select-Object CommandLine | fl`, { shell: 'powershell' }]
        : [`ps x -o args | grep '${name}'`]

...

const { stdout } = await exec(...args)
junlarsen commented 2 years ago

Is this going to cause issues with non windows systems? Maybe a better option would be something like

const args =
      process.platform === 'win32'
        ? [`Get-CimInstance -Query "SELECT * from Win32_Process WHERE name LIKE '${name}.exe'" | Select-Object CommandLine | fl`, { shell: 'powershell' }]
        : [`ps x -o args | grep '${name}'`]

...

const { stdout } = await exec(...args)

Oh you're right about that, I believe a solution like you've proposed here is better. Thanks!

junlarsen commented 2 years ago

Hi, any updates on this? Looking forward to merging it!

Nerdsie commented 2 years ago

Hi, any updates on this? Looking forward to merging it!

Sorry, was busy with work. Just pushed changes, not exactly what I had in my comment because type issues were wonky. Is there a better way to run tests than running jest manually from cli? Seems like there should be a "test" package.json script

junlarsen commented 2 years ago

Sorry, was busy with work. Just pushed changes, not exactly what I had in my comment because type issues were wonky. Is there a better way to run tests than running jest manually from cli? Seems like there should be a "test" package.json script

Again, thank you so much for the changes. And yes, you're right, I should totally have a test package.json script for this, will add one.

E: Looks like I had a change for exactly that locally, pushed to master in a901c14a6531829ff32eacd6df7a94843c5a96ef

junlarsen commented 2 years ago

Thank you so much for your work, I've released 5.3.1 with your patch https://www.npmjs.com/package/league-connect