fix(cli): use a shared cross-platform npm process resolver without shell#2257
fix(cli): use a shared cross-platform npm process resolver without shell#2257ghostdevv merged 7 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Dam I got nerd sniped! Lmk if ya need a test on Windows (if you're not also on Windows / have a windows machine) |
|
if you have a windows machine, go ahead, more the testers the better. I don't have one, so was planning to ask my friend today evening, will mention how that went. |
|
Checking it out now, are you in the discord? @MathurAditya724 there is a few issues with your implementation. |
|
yes, I'm |
|
As stated on Discord, the changes you made seemed to have caused some issues; my output with your changesPS F:\GitHub\temp\npmx.dev\cli> npmx-connector
┌ npmx connector
│
▲ This allows npmx.dev to access your npm cli and any authenticated contexts.
│
◇ Do you accept?
│ Yes
│
● Checking npm authentication...
│
▲ Not logged in to npm. Starting npm login...
Error: spawn EINVAL
at ChildProcess.spawn (node:internal/child_process:421:11)
at spawn (node:child_process:796:9)
at file:///F:/GitHub/temp/npmx.dev/cli/dist/cli.mjs:16:17
at new Promise (<anonymous>)
at runNpmLogin (file:///F:/GitHub/temp/npmx.dev/cli/dist/cli.mjs:15:9)
at Object.run (file:///F:/GitHub/temp/npmx.dev/cli/dist/cli.mjs:54:31)
at process.processTicksAndRejections (node:internal/process/task_queues:104:5)
at async runCommand (file:///F:/GitHub/temp/npmx.dev/node_modules/.pnpm/citty@0.2.1/node_modules/
citty/dist/index.mjs:188:47)
at async runMain (file:///F:/GitHub/temp/npmx.dev/node_modules/.pnpm/citty@0.2.1/node_modules/cit
ty/dist/index.mjs:285:10) {
errno: -4071,
code: 'EINVAL',
syscall: 'spawn'
}I asked Claude to fix up the issues and this was the Fix that the model proposed and I tested it manually and seems to work as intended; Fix suggested by Claude Opus 4.6Lines 15, through 21// ~/cli.ts
-const NPM_COMMAND = process.platform === 'win32' ? 'npm.cmd' : 'npm'
-
async function runNpmLogin(): Promise<boolean> {
return new Promise(resolve => {
- const child = spawn(NPM_COMMAND, ['login', `--registry=${NPM_REGISTRY_URL}`], {
- stdio: 'inherit',
- })
+
+ const loginArgs = ['login', `--registry=${NPM_REGISTRY_URL}`]
+ const child =
+ process.platform === 'win32'
+ ? spawn('cmd.exe', ['/c', 'npm', ...loginArgs], { stdio: 'inherit' })
+ : spawn('npm', loginArgs, { stdio: 'inherit' })Routing through Might be good to get some more eyes on it though. |
|
Seems to work as intended now; Whilst Logged in to npmjs;PS F:\GitHub\temp\npmx.dev\cli> bun run build
$ tsdown
ℹ tsdown v0.21.4 powered by rolldown v1.0.0-rc.9
ℹ config file: F:\GitHub\temp\npmx.dev\cli\tsdown.config.ts
ℹ entry: src\index.ts, src\cli.ts
ℹ target: node24.0.0
ℹ tsconfig: tsconfig.json
WARN `external` is deprecated. Use `deps.neverBundle` instead.
ℹ Build start
ℹ Granting execute permission to dist\cli.mjs
ℹ dist\cli.mjs 2.62 kB │ gzip: 1.23 kB
ℹ dist\index.mjs 0.17 kB │ gzip: 0.14 kB
ℹ dist\server-FP9KQp94.mjs.map 81.93 kB │ gzip: 18.58 kB
ℹ dist\server-FP9KQp94.mjs 40.86 kB │ gzip: 9.61 kB
ℹ dist\cli.mjs.map 4.53 kB │ gzip: 1.91 kB
ℹ dist\index.d.mts.map 2.16 kB │ gzip: 0.67 kB
ℹ dist\node-pty-PmtOFpde.d.mts.map 1.45 kB │ gzip: 0.65 kB
ℹ dist\index.d.mts 5.20 kB │ gzip: 1.60 kB
ℹ dist\cli.d.mts 0.01 kB │ gzip: 0.03 kB
ℹ dist\node-pty-PmtOFpde.d.mts 1.52 kB │ gzip: 0.59 kB
ℹ 10 files, total: 140.45 kB
✔ Build complete in 678ms
PS F:\GitHub\temp\npmx.dev\cli> bun install
bun install v1.3.11 (af24e281)
Checked 57 installs across 83 packages (no changes) [11.00ms]
PS F:\GitHub\temp\npmx.dev\cli> bun run build
$ tsdown
ℹ tsdown v0.21.4 powered by rolldown v1.0.0-rc.9
ℹ config file: F:\GitHub\temp\npmx.dev\cli\tsdown.config.ts
ℹ entry: src\index.ts, src\cli.ts
ℹ target: node24.0.0
ℹ tsconfig: tsconfig.json
WARN `external` is deprecated. Use `deps.neverBundle` instead.
ℹ Build start
ℹ Cleaning 11 files
ℹ Granting execute permission to dist\cli.mjs
ℹ dist\cli.mjs 2.62 kB │ gzip: 1.23 kB
ℹ dist\index.mjs 0.17 kB │ gzip: 0.14 kB
ℹ dist\server-FP9KQp94.mjs.map 81.93 kB │ gzip: 18.58 kB
ℹ dist\server-FP9KQp94.mjs 40.86 kB │ gzip: 9.61 kB
ℹ dist\cli.mjs.map 4.53 kB │ gzip: 1.91 kB
ℹ dist\index.d.mts.map 2.16 kB │ gzip: 0.67 kB
ℹ dist\node-pty-PmtOFpde.d.mts.map 1.45 kB │ gzip: 0.65 kB
ℹ dist\index.d.mts 5.20 kB │ gzip: 1.60 kB
ℹ dist\cli.d.mts 0.01 kB │ gzip: 0.03 kB
ℹ dist\node-pty-PmtOFpde.d.mts 1.52 kB │ gzip: 0.59 kB
ℹ 10 files, total: 140.45 kB
✔ Build complete in 627ms
PS F:\GitHub\temp\npmx.dev\cli> bun link
bun link v1.3.11 (af24e281)
Success! Registered "npmx-connector"
To use npmx-connector in a project, run:
bun link npmx-connector
Or add it in dependencies in your package.json file:
"npmx-connector": "link:npmx-connector"
PS F:\GitHub\temp\npmx.dev\cli> npmx-connector
┌ npmx connector
│
▲ This allows npmx.dev to access your npm cli and any authenticated contexts.
│
◇ Do you accept?
│ Yes
│
● Checking npm authentication...
│
● Authenticated as: mynameistito
│
◇ Click to connect ──────────────────────────────────────────────────────────╮
│ │
│ Open: https://npmx.dev/?token=$TOKEN&port=31415 │
│ │
│ Or paste token manually: $TOKEN │
│ │
├─────────────────────────────────────────────────────────────────────────────╯
➜ Listening on: http://127.0.0.1:31415/
│
● Waiting for connection... (Press Ctrl+C to stop)
Server closed successfully.
PS F:\GitHub\temp\npmx.dev\cli>Without being logged into npmjs;PS F:\GitHub\temp\npmx.dev\cli> npmx-connector
┌ npmx connector
│
▲ This allows npmx.dev to access your npm cli and any authenticated contexts.
│
◇ Do you accept?
│ Yes
│
● Checking npm authentication...
│
▲ Not logged in to npm. Starting npm login...
npm notice Log in on https://registry.npmjs.org/
Login at:
https://www.npmjs.com/login?next=/login/cli/$UUID
Press ENTER to open in the browser...
/However, there was an issue when trying to originally build on -export function createConnectorApp(expectedToken: string) {
+export function createConnectorApp(expectedToken: string): H3 { |
|
Thanks for tackling this! Directly spawning I’d recommend using |
MathurAditya724
changed the title
|
@RYGRIT just changed the approach, thanks to @mynameistito, and update the PR title and description to better reflect what we have done |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new utility Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cli/src/npm-client.ts (1)
336-337: Please route the PTY path through the same resolver as well.These two call sites now use
resolveNpmProcessCommand(), butexecNpmInteractive()still hardcodespty.spawn('npm', npmArgs, ...)at Line 213. On Windows, Node documents that.cmdentrypoints need a terminal/cmd.exe, and node-pty’s own Windows example launches a shell rather than a raw command, so the interactive OTP/browser-auth branch is still on a different launcher. Please wire it through the same resolver, or an equivalent PTY-safe wrapper, so Windows behaviour stays consistent. (nodejs.org)Also applies to: 611-612
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e08d4647-5925-49aa-87af-4b52453a8238
📒 Files selected for processing (3)
cli/src/cli.tscli/src/npm-client.tscli/src/npm-process.ts
| if (platform === 'win32') { | ||
| return { | ||
| command: comSpec || 'cmd.exe', | ||
| args: ['/d', '/s', '/c', 'npm', ...npmArgs], | ||
| } | ||
| } |
There was a problem hiding this comment.
Are there any examples anyone is aware of, of other projects doing this? Just want to make sure, as not a windows user, this is the safest best way 😅
There was a problem hiding this comment.
unsure if it's the safest way, however am aware that it works as intended to minimise the DEP1090 Issue
There was a problem hiding this comment.
Looked into this a bit more.
The two approaches I found were either:
- keep using a shell, but pass a single command string
- explicitly use
cmd.exe /d /s /c ...on Windows
Node-RED took the first approach for the same deprecation, with the main change in b364f8f. For this case though, I think the second one is the better fit. It is still consistent with Node’s Windows guidance for spawning .bat and .cmd files, while avoiding shell: true with args[], which is what DEP0190 warns about.
So this seems like a solid approach to me.
4 participants
Summary
Fixes the Windows connector auth flow while avoiding shell-based npm execution and DEP0190 warnings.
What changed
cli/src/npm-process.ts.cli/src/cli.ts(npm login)cli/src/npm-client.ts(non-interactive npm exec + package publish path)npmdirectly.cmd.exe /d /s /c npm ...(noshell: true, no directnpm.cmdspawn).Why
The previous approach removed DEP0190 warnings but caused Windows runtime failures (
spawn EINVAL) in login flow.Using a centralized, explicit cross-platform command resolver keeps behavior consistent and avoids shell-based execution.
Closes #2216