Vscode-js-debug: inject NODE_OPTIONS will cause Node 10.x crash when fork child process

[atian25]

Describe the bug

some users of our project report an issue, when running cnpm install at vscode, it will crash.

Error: Command failed: /Users/sunny/Documents/Workspace/My/dingtalk-robot-server/node_modules/node/bin/node -e "process.stdout.write(JSON.stringify(process.versions))"
internal/modules/cjs/loader.js:638
    throw err;
    ^

Error: Cannot find module '"/Users/sunny/Library/Application'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:636:15)
    at Function.Module._load (internal/modules/cjs/loader.js:562:25)
    at Module.require (internal/modules/cjs/loader.js:692:17)

$ echo ${NODE_OPTIONS}
--require "/Users/sunny/Library/Application Support/Code/User/workspaceStorage/0566b62ed6c96028f6eca701b9ac9d1e/ms-vscode.js-debug/bootloader.js"

To Reproduce
Steps to reproduce the behavior:

after a long dig, we found it seems a bug of Node 10.x

see https://github.com/atian25/node-bug-report

just a notify for you, free to close this issue.

Log File

VS Code Version: Replace me!

Additional context
Add any other context about the problem here.

[connor4312]

We only will quote the path if we see Node 12 or higher on your PATH. I see that your Node binary is actually set to node_modules/node/bin/node, which is weird. How do you get that setup, do you use a version manager for this?

You can set nodeVersonHint: 10 in your launch.json with the nightly build to override/set the detected version.

[iFwu]

FYI, related node issue nodejs/node#12971, vscode issue microsoft/vscode#103049 and StackOverflow question: https://stackoverflow.com/questions/62922875/vs-code-integrated-terminal-throws-cannot-find-module-nvm-is-not-compatible

[iFwu]

Change Node.js version lower than 12 in PATH without re-toggling the autoAttachFilter option will cause this issue.

[atian25]

@connor4312 it just a showcase for us to report the issue.

in our case, the user just open vscode terminal (when their global node version is 10.x), then exec cnpm install which will fork a child process, then they crash.

it's not about something like debug and launch.json.

[connor4312]

When their global Node version is 10, we will detect that and format arguments correctly for Node 10.

[atian25]

got, but at our use-case, the node version comes with the project itself.

just using something like npm install node --save

and when exec npm run some-scripts the node_modules/.bin will auto attach to PATH, so the execPath is target to special node.

[connor4312]

Thanks for the info. I think I will add some logic to our binary provider to try to detect that case.

[connor4312]

This will be fixed in the next nightly build; when running an npm script we will now prepend node_modules/.bin to the path for discovery.

[atian25]

could you add cnpm and tnpm to the package manager list? it's the most popular fork of npm at china due to the GFW.

[connor4312]

Done 👍

[iFwu]

@connor4312 I think there's still a problem after switching the node version to v10 without changing the autoAttachFilter flag. Do I need to open another issue for it?

[connor4312]

Auto attach is a separate issue... but I'm not sure there's any action we will want to take for that. The large majority of users are on Node 12 or higher, and those that aren't generally don't downgrade to 10. We are also not able to detect a downgrade without actively polling, which I would really want to avoid.

[rickyes]

Paths containing spaces and double quotes are already supported in Node.js v10, Release PR: https://github.com/nodejs/node/pull/35544