Change in NODE_OPTIONS parsing behavior in Node 11

[rwjblue]

• Version: v11.9.0
• Platform: Darwin
• Subsystem:

If NODE_OPTIONS starts with a leading space the --require option does not function (possibly other flags but I've only tested --require).

Specifically:

NODE_OPTIONS="--require ./some-file.js" node ./other-file.js

^ will properly require ./some-file.js prior to executing ./other-file.js

Whereas:

NODE_OPTIONS=" --require ./some-file.js" node ./other-file.js

^^ will not require ./some-file.js at all

Steps to reproduce:

mkdir test-node-options
cd test-node-options
echo "console.log('first!')" > first.js
echo "console.log('second!')" > second.js

Then compare the output of these two commands:

# "broken"
NODE_OPTIONS=" --require ./first.js" node ./second.js

# "working"
NODE_OPTIONS="--require ./first.js" node ./second.js

---

I stumbled across this while attempting to use Yarn's PnP system on Node 11 (see yarnpkg/yarn#7092 for the original report there). Yarn 1.13.0 adds --require ${PATH_TO_PNP_FILE} to NODE_OPTIONS which causes this issue.

/cc @arcanis @Turbo87 @stefanpenner

[arcanis]

Wow 😮 The most obvious suspect would be 8ec3c35

Probably a good time to put #24065 back on the table as well

[addaleax]

@arcanis Do you want to continue work on your PR and fix this along with it? Otherwise we can do a new PR for this specific problem?

[arcanis]

I'll give it a look this evening (I might have to revert part of 8ec3c35 since it wouldn't make sense to use the same SplitString anymore?) - if no progress by the end of the week feel free to fix it separately 🙂

[addaleax]

@arcanis I think both consumers of SplitString() would like to skip empty items (which I assume is the issue here), so I think it’s okay to modify the common code. But yeah, reverting it should also be fine.

[stefanpenner]

@rwjblue thanks for debugging this one!

[himself65]

Can we make SplitString() to trim white spaces?

[himself65]

maybe we should ignore white spaces like this on SplitString()

    if (item.empty()) continue;

[arcanis]

I've rebased, updated, and fixed this issue in #24065

[himself65]

fixed on 17ab2ed

[stefanpenner]

@himself65 / @arcanis is the plan to ship this as part of an upcoming node@11.x.y release?

[himself65]

@himself65 / @arcanis is the plan to ship this as part of an upcoming node@11.x.y release?

i don’t know :(

you can ask the Node Team Members about it.

[stefanpenner]

cc @addaleax ^

[lpinca]

@stefanpenner it seems this shipped with Node.js 11.12.0. I'm going to close this. Please comment back if needed.