Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 12, 2025

Conversation

alexsch01
Copy link
Contributor

@alexsch01 alexsch01 commented May 7, 2025

Requires #8297 and #8303 after


This fixes the command npm test -- hello -p1 world -p2 "hello world" --q1=hello world --q2="hello world" in Windows PowerShell and pwsh7

  • where the "test" script prints all the arguments passed after the first "--" in the command above

Before this change

PS> npm test -- hello -p1 world -p2 "hello world" --q1=hello world --q2="hello world"
npm warn "world" is being parsed as a normal command line argument.
npm warn "hello world" is being parsed as a normal command line argument.
npm warn Unknown cli config "--p1". This will stop working in the next major version of npm.
npm warn Unknown cli config "--p2". This will stop working in the next major version of npm.
npm warn Unknown cli config "--q1". This will stop working in the next major version of npm.
npm warn Unknown cli config "--q2". This will stop working in the next major version of npm.

> [email protected] test
> node args.js hello world hello world world

hello
world
hello world
world

With this change

PS> npm test -- hello -p1 world -p2 "hello world" --q1=hello world --q2="hello world"

> [email protected] test
> node args.js hello -p1 world -p2 hello world --q1=hello world --q2=hello world

hello
-p1
world
-p2
hello world
--q1=hello
world
--q2=hello world

Also, fixes comma-separated values in Windows PowerShell and pwsh7

Before this change

PS> npm help a=1,b=2,c=3
No matches in help for: a=1 b=2 c=3

With this change

PS> npm help a=1,b=2,c=3
No matches in help for: a=1,b=2,c=3

Copy link
Contributor

@mbtools mbtools left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Thanks again for your effort!

@alexsch01
Copy link
Contributor Author

alexsch01 commented May 7, 2025

Don't know when npm 11 series will be bundled with Nodejs 20/22 but would it be possible for this to get backported to npm 10 series?

@wraithgar
Copy link
Member

Getting it into npm 10 would simply be a matter of opening a PR into release/v10.

Getting that backported into node is something we are working on. The script that does this is currently failing and needs attention.

@wraithgar
Copy link
Member

At this point npm 11 will not be in node 20/22

wraithgar
wraithgar previously approved these changes May 7, 2025
@wraithgar
Copy link
Member

wraithgar commented May 7, 2025

A collaborator needs to approve PRs before they can be landed, so I have done that. My approval was wholly dependent on @mbtools approval.

Thanks to everyone who participated in this, it was not a trivial task. I will merge this later today unless someone finds something egregious that you all missed.

@alexsch01
Copy link
Contributor Author

alexsch01 commented May 8, 2025

At this point npm 11 will not be in node 20/22

Ahh I forgot that node 20 including this change doesn't matter since ps1 scripts aren't there in that release

@alexsch01
Copy link
Contributor Author

alexsch01 commented May 8, 2025

needed to make a change to escape grave key " ` " in directories since that's a valid key in Windows

@mbtools @wraithgar please review, this was the last character I was worried about and the latest commit fixes it

@mbtools
Copy link
Contributor

mbtools commented May 8, 2025

lgtm 👍

@wraithgar
Copy link
Member

Waiting on #8280 to land first so that CI goes back to green.

@alexsch01
Copy link
Contributor Author

Once this PR lands, I'll use git cherry-pick to make a backport PR for v10

@wraithgar
Copy link
Member

The easiest path to landing this in release/v10 would be to wait for this PR to land (since it'll be squashed into a single commit), then cherry-picking it into a new commit on the v10 branch and making a new PR.

Please be aware that the current node PR script is not working to make backports, that's on our list of things to triage so that we can get node 22 up to date w/ the latest npm 10.

@wraithgar wraithgar merged commit 2210d7a into npm:latest May 12, 2025
20 checks passed
@github-actions github-actions bot mentioned this pull request May 12, 2025
@alexsch01 alexsch01 deleted the latest-2 branch May 12, 2025 16:57
alexsch01 added a commit to alexsch01/cli that referenced this pull request May 12, 2025
Continuation of npm#8267 @mbtools 

---

This fixes the command `npm test -- hello -p1 world -p2 "hello world"
--q1=hello world --q2="hello world"` in Windows PowerShell and pwsh7
- where the "test" script prints all the arguments passed after the
first "--" in the command above

Before this change

```
PS> npm test -- hello -p1 world -p2 "hello world" --q1=hello world --q2="hello world"
npm warn "world" is being parsed as a normal command line argument.
npm warn "hello world" is being parsed as a normal command line argument.
npm warn Unknown cli config "--p1". This will stop working in the next major version of npm.
npm warn Unknown cli config "--p2". This will stop working in the next major version of npm.
npm warn Unknown cli config "--q1". This will stop working in the next major version of npm.
npm warn Unknown cli config "--q2". This will stop working in the next major version of npm.

> [email protected] test
> node args.js hello world hello world world

hello
world
hello world
world
```

With this change

```
PS> npm test -- hello -p1 world -p2 "hello world" --q1=hello world --q2="hello world"

> [email protected] test
> node args.js hello -p1 world -p2 hello world --q1=hello world --q2=hello world

hello
-p1
world
-p2
hello world
--q1=hello
world
--q2=hello world
```

---

Also, fixes comma-separated values in Windows PowerShell and pwsh7

Before this change

```
PS> npm help a=1,b=2,c=3
No matches in help for: a=1 b=2 c=3
```

With this change

```
PS> npm help a=1,b=2,c=3
No matches in help for: a=1,b=2,c=3
```
@alexsch01 alexsch01 mentioned this pull request May 12, 2025
@alexsch01
Copy link
Contributor Author

Fixes #7375

@noseratio
Copy link

Thanks heaps @alexsch01 and everyone else involved!

@alexsch01
Copy link
Contributor Author

alexsch01 commented May 15, 2025

FYI: this PR should be followed up with #8297 and #8300 in terms of backporting

@alexsch01
Copy link
Contributor Author

alexsch01 commented May 15, 2025

@wraithgar just want to let you know in case anyone is trying this by doing npm i -g npm on their NodeJS install.....
this change will not fix that case since that will land in the %AppData%\npm directory

It has to be installed in the NodeJS directory which I manually tested worked

alexsch01 added a commit to alexsch01/cli that referenced this pull request May 15, 2025
Continuation of npm#8267 @mbtools 

---

This fixes the command `npm test -- hello -p1 world -p2 "hello world"
--q1=hello world --q2="hello world"` in Windows PowerShell and pwsh7
- where the "test" script prints all the arguments passed after the
first "--" in the command above

Before this change

```
PS> npm test -- hello -p1 world -p2 "hello world" --q1=hello world --q2="hello world"
npm warn "world" is being parsed as a normal command line argument.
npm warn "hello world" is being parsed as a normal command line argument.
npm warn Unknown cli config "--p1". This will stop working in the next major version of npm.
npm warn Unknown cli config "--p2". This will stop working in the next major version of npm.
npm warn Unknown cli config "--q1". This will stop working in the next major version of npm.
npm warn Unknown cli config "--q2". This will stop working in the next major version of npm.

> [email protected] test
> node args.js hello world hello world world

hello
world
hello world
world
```

With this change

```
PS> npm test -- hello -p1 world -p2 "hello world" --q1=hello world --q2="hello world"

> [email protected] test
> node args.js hello -p1 world -p2 hello world --q1=hello world --q2=hello world

hello
-p1
world
-p2
hello world
--q1=hello
world
--q2=hello world
```

---

Also, fixes comma-separated values in Windows PowerShell and pwsh7

Before this change

```
PS> npm help a=1,b=2,c=3
No matches in help for: a=1 b=2 c=3
```

With this change

```
PS> npm help a=1,b=2,c=3
No matches in help for: a=1,b=2,c=3
```
@alexsch01 alexsch01 mentioned this pull request May 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants