cli: NODE_OPTIONS does not support --require/-r + path with spaces

[vsemozhetbyt]

• Version: v8.0.0 nightly 2017 05 07
• Platform: Windows 7 x64
• Subsystem: cli

Refs: #12028

test.js:

'use strict';

console.log('B');

module.js:

console.log('A');

"mo dule.js":

console.log('A');

Compare:

> node -r ./module.js test.js
A
B

> set NODE_OPTIONS=-r ./module.js

> set NODE_OPTIONS
NODE_OPTIONS=-r ./module.js

> node test.js
A
B

> node -r "./mo dule.js" test.js
A
B

> set NODE_OPTIONS=-r "./mo dule.js"

> set NODE_OPTIONS
NODE_OPTIONS=-r "./mo dule.js"

> node test.js
node: dule.js" is not supported in NODE_OPTIONS

[vsemozhetbyt]

Due to this issue, https://github.com/nodejs/node/blob/master/test/parallel/test-cli-node-options.js fails with spaces in the path to repo root and this cannot be fixed in the test.

cc @sam-github

[sam-github]

I punted on this because I can't see any credible use-case for module names that have spaces in them. However, I didn't consider that Windows occaisonally forces/encourages top-level directory names with spaces in them.

This can be fixed in the test, its choosing to use absolute paths ATM, that's not necessary, relative paths would work fine. I'll rework the test, first.

@vsemozhetbyt do you think its really necessary to add a full-blown argv parser in C++ that understands ', " (and \', \")?

[vsemozhetbyt]

@sam-github Yes, it is not the name only, it is the same with spaces in any folder during the module path. I've just use space in the name for an easier repro.

Unfortunately, I do not know C++ and cannot understand all the implementation burden. But now we have a different behavior of cli -r and NODE_OPTIONS -r. If we do not plan to complete this behavior, maybe we should document the difference.

[sam-github]

The difference is the shell, shells parse the quotes (not the node CLI), but with env vars there is no shell, so some subset of what the shell does would need to also be implemented by node. Which is doable.

[vsemozhetbyt]

Well, maybe this is not worth it. I've just stumble upon this fixing #12773. Feel free to close if this is of lowest priority.

[sam-github]

Its an issue, I'll look at it sometime. It would be more important if people run into it in practice, but I'm not sure they will. In your particular case, I can't even build node, much less run the tests, see #12773 (comment)

Having modules with spaces in their name my module.js is plainly weird, and its not necessary usually to provide full paths to modules (node -r /a/path/"with spaces"/module.js).

[Trott]

Should this remain open?

[richardlau]

Should this remain open?

Probably, since #21575 has just been opened about the same issue.

[arcanis]

It's likely this issue will appear a bit more. Yarn will use NODE_OPTIONS to properly setup the PnP environment (at least until the package-level loaders are ready for consumption), meaning that people having spaces into their home directories might have problems.

Note that imo supporting a full-blown parser with both ' and " isn't necessary. Supporting \ would be enough to at least make it possible to add spaces, and would have a very simple implementation.

[arcanis]

I've opened #24065 with a fix.