fix(powershell): use Invoke-Expression to pass args

[lukekarrys]

Co-authored-by: @noseratio [email protected]

[wraithgar]

We should be absolutely sure this won't overlap w/ npm's escaping when it spawns scripts.

[lukekarrys]

Converted to draft as I’m still experimenting with this.

[noseratio]

@lukekarrys also please don't use MyInvocation.Line, use MyInvocation.Statement instead. Here's why:

If npm.ps1 is invoked as part of a multi-statement one-liner, like echo "Hello" && npm start -- commmand --arg=value, then MyInvocation props would look like this:

  "Line": "echo \"Hello\" && npm start -- commmand --arg=value",
  "Statement": "npm start -- commmand --arg=value",
  "InvocationName": "npm",

PS Thanks for a shoutout! 🙂

[lukekarrys]

Thanks for the tip @noseratio. I used MyInvocation.Line in a force-push after I couldn't get MyInvocation.Statement working. In the tests these files are called directly via child_process.spawn so I'm not sure if that is affecting them. I will need to do some more research to see why both Line and Statement are not available on MyInvocation in tests.

[lukekarrys]

But MyInvocation.Statement did work for me locally.

[noseratio]

Also interestingly, if npm is typed in a Git-Bash prompt on Windows, the Bash version - "C:\Program Files\nodejs\npm" - will get invoked.

[noseratio]

Last but not least, npm.cmd remains being super important, as it will be invoked for nested npm scripts (like startapp below), regardless of the outer shell:

package.json:

{
  "name": "cli-test",
  "version": "1.0.0",
  "main": "index.js",
  "scripts": {
    "start": "node index.js",
    "startapp": "npm start -- ",
    "showcli": "echo"
  }
}

C:\temp\cli-test>pwsh –noprofile                                                                                                      PowerShell 7.4.2
PS C:\temp\cli-test> npm run startapp -- command --arg=value
Hello from npm.ps1

> [email protected] startapp
> npm start -- command --arg=value
Hello from npm.bat

> [email protected] start
> node index.js command --arg=value

[
  'C:\\Program Files\\nodejs\\node.exe',
  'C:\\temp\\cli-test\\index.js',
  'command',
  '--arg=value'
]
PS C:\temp\cli-test> & "C:\Program Files\Git\bin\bash.exe"

Master@i3msi MINGW64 /c/temp/cli-test
$ npm run startapp -- command --arg=value
Hello from npm[.sh]

> [email protected] startapp
> npm start -- command --arg=value

Hello from npm.bat

> [email protected] start
> node index.js command --arg=value

[
  'C:\\Program Files\\nodejs\\node.exe',
  'C:\\temp\\cli-test\\index.js',
  'command',
  '--arg=value'
]

That can probably only be changed by overriding the shell in .npmrc:

shell = "pwsh"

[alexsch01]

@noseratio

also please don't use MyInvocation.Line, use MyInvocation.Statement instead.

On my Windows PowerShell, MyInvocation.Statement doesn't exist.
In #8267, I chose to use MyInvocation.Line due to that

[mbtools]

This needs to be conditional. "Line" does not work when multiple statements are joined which is common like " echo 'test' & npm run my-script".

An additional challenge is how we test using spawn. In my quick try, neither $MyInnvocation.Statement nor Line are populated.

We can get this done but it will take some tries.

[alexsch01]

@mbtools fixed multiple statements in latest commit of the new PR
unforunately $MyInvocation.Position is not populated either

[noseratio]

@lukekarrys Node v22.1.0 was out, and I had to patch "C:\Program Files\nodejs\npm.ps1" again.

My current version:

#!/usr/bin/env pwsh

$NODE_EXE="$PSScriptRoot/node.exe"
if (-not (Test-Path $NODE_EXE)) {
  $NODE_EXE="$PSScriptRoot/node"
}
if (-not (Test-Path $NODE_EXE)) {
  $NODE_EXE="node"
}

$NPM_PREFIX_JS="$PSScriptRoot/node_modules/npm/bin/npm-prefix.js"
$NPM_CLI_JS="$PSScriptRoot/node_modules/npm/bin/npm-cli.js"
$NPM_PREFIX=(& $NODE_EXE $NPM_PREFIX_JS)

if ($LASTEXITCODE -ne 0) {
  Write-Host "Could not determine Node.js install directory"
  exit 1
}

$NPM_PREFIX_NPM_CLI_JS="$NPM_PREFIX/node_modules/npm/bin/npm-cli.js"
if (Test-Path $NPM_PREFIX_NPM_CLI_JS) {
  $NPM_CLI_JS=$NPM_PREFIX_NPM_CLI_JS
}

function Normalize {
    [CmdletBinding()]
    param(
        [Parameter(Mandatory=$true, ValueFromPipeline=$true, Position=0)]
        [string]$Path
    )

    $Path = [System.IO.Path]::GetFullPath($Path)
    # remove trailing " or ' quotes (if any) and put back " quotes around the path
    $Path = $Path -replace '^\s*"\s*(.*?)\s*"\s*$', '$1'
    $Path = $Path -replace "^\s*'\s*(.*?)\s*'\s*$", "$1"
    return """$Path"""
}

$NPM_ARGS = $MyInvocation.Statement.Substring($MyInvocation.InvocationName.Length).Trim()
$INVOKE_NPM = "& $(Normalize $NODE_EXE) $(Normalize $NPM_CLI_JS) $NPM_ARGS"
                                           
# Support pipeline input
if ($MyInvocation.ExpectingInput) {
  $input | Invoke-Expression $INVOKE_NPM
} else {
  Invoke-Expression $INVOKE_NPM
}

exit $LASTEXITCODE

[mbtools]

Andrew's solution is working well.

Let's merge the last version above and fix this annoying bug.

The logs expired. Luke @lukekarrys, could you please update the PR and trigger CI so we see what's missing?

Thank you 🙏

[lukekarrys]

I won't be able to push this over the finish line. It's probably best if a new PR is opened with the proposed solution.

[noseratio]

Andrew's solution is working well.

@mbtools , thanks for vouching! I currently re-apply this patch semi-automatically everytime I upgrade node.js on Windows:
#7375 (comment)

I'm sorry I don't have capacity right now to re-opon this PR, but I'd be grateful if anyone could pick it up.

[alexsch01]

@noseratio I used your newer patch to open #8267, and did the same for npx.ps1
Thanks for the patch!

EDIT: I changed $MyInvocation.Statement to $MyInvocation.Line since Statement doesn't work in Windows PowerShell

[mbtools]

There are differences between Windows PowerShell, PowerShell before 7.3 and PowerShell >= 7.3. We need to be clear what works and what doesn't (see my comment in the PR)