repl / eval: CommonJS globals leak into ESM modules

[coreyfarrell]

• Version: v14.0.0-pre / cf5ce2c
• Platform: Linux lt2.cfware.com 5.3.11-200.fc30.x86_64 #1 SMP Tue Nov 12 19:25:25 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem: repl, CLI --eval and --print without --input-type=module

Create script.mjs:

console.log('script.mjs', typeof require);

Running node ./script.mjs or node --input-type=module --eval "import('./script.mjs')" both produce the correct output script.mjs undefined.

Now run import('./script.mjs') in repl, this produces output script.mjs function. Same for node --eval "import('./script.mjs')".

Using --print in place of --eval does not change typeof require.

CC @nodejs/modules-active-members

[devsnek]

I think the only way to fix this would be some trickery with virtual with contexts (like the context_extensions option of CompileFunctionInContext). @hashseed does adding such an option to normal script compilation seem reasonable?

[hashseed]

Why is this an issue? Imo repl does not have to be spec compliant. Also there is no spec for require.

[ljharb]

@hashseed note that according to the OP it happens for node --eval as well, so it's not just the repl.

[devsnek]

Imo repl does not have to be spec compliant. Also there is no spec for require.

i mean ideally we want to avoid cjs locals leaking into modules (and other places). if v8 is not willing to accept such a change we can call this "won't fix" (and i won't be that broken up about it) but imo it would be nice to fix.

[jkrems]

note that according to the OP it happens for node --eval as well, so it's not just the repl.

I think you misread the first paragraph: It sounds like in the --eval case, the module logs undefined as expected. This only happens in the repl.

@devsnek Would it be possible to use the RuntimeAgent.evaluate for the repl with require from the console API instead from global? That should prevent it from leaking into the global scope and it wouldn't appear in "normal" code while still working in the repl expressions. That would also move it closer to the way the repl in devtools/debug clients works.

[coreyfarrell]

@jkrems that's not quite true. --eval with --input-type=module does the right thing but the commonjs variables are leaked when node --eval is used without --input-type=module.

[jkrems]

Oh, we set them as globals there, too? That’s definitely weird. Don’t see why we need for —eval.

[coreyfarrell]

For --eval and --print run without --input-type=module they're set here:
https://github.com/nodejs/node/blob/master/lib/internal/process/execution.js#L75-L79

Inside script the commonjs variables are function arguments created by vm.compileFunction so I assume they're being copied to global to become available inside vm.runInThisContext.

[jkrems]

I wish there was a comment in that file explaining why we're not using the typical function wrapper. Is it "because it would be a breaking change" at this point..? First level of blame doesn't show anything obvious.

[devsnek]

@jkrems --print 1 needs to print 1, so wrapping it in a function wouldn't work.

[jkrems]

Gotcha, thanks for the explanation! But v8-inspector and console API may be a possible path for --print/--eval I assume?

[devsnek]

@jkrems inspector doesn't introduce new magic scope. in fact, that would also be an issue with the virtual with scope, so i guess that won't work either...

with repl we can transform the code arbitrarily and no one will really care, so we can fix it with enough effort, but for --eval we need to keep everything exact, and i can't think of a way to make it work.

[jkrems]

inspector doesn't introduce new magic scope.

Ah, the console API is also available for all other code running, not just the evaluated expression? That's too bad. :( The upside is that it gets automatically removed once the initial evaluation stops (doesn't survive to future ticks) but I'm not sure if that's enough for this..? As long as ESM doesn't run in the same tick, it wouldn't see the require anymore.

[guybedford]

Since there is no viable route forward here, and the design decisions in V8 encapsulation and the Node.js REPL have been made, closing this as a "wontfix".

[devsnek]

This is still on my list of things to fix if ever possible