JS source should not be wrapped.

@schuay

When Node.js executes JavaScript, it internally wraps them into an IIFE:

Module.wrap = function(script) {
  return Module.wrapper[0] + script + Module.wrapper[1];
};

Module.wrapper = [
  '(function (exports, require, module, __filename, __dirname) { ',
  '\n});'
];

There are some problems with this:

Invalid JavaScript is parsed as correct. This following example executes just fine:

});

(function() {
  console.log(1);
})();

(function() {

This wrapper shows up as part of source in the DevTools. Since the start line and column are passed as 0, the source positions of errors and break points at least for the first line of the script are off:

$ cat test.js
throw new Error();

$ node test.js
/usr/local/google/home/yangguo/node/test.js:1
(function (exports, require, module, __filename, __dirname) { throw new Error();
                                                              ^
Error
    at Object.<anonymous> (/usr/local/google/home/yangguo/node/test.js:1:69)
    at Module._compile (module.js:573:30)
    at Object.Module._extensions..js (module.js:584:10)
    at Module.load (module.js:507:32)
    at tryModuleLoad (module.js:470:12)
    at Function.Module._load (module.js:462:3)
    at Function.Module.runMain (module.js:609:10)
    at startup (bootstrap_node.js:158:16)
    at bootstrap_node.js:578:3

This disparity between actual source and source being passed to V8 also affects code coverage. See https://bugs.chromium.org/p/v8/issues/detail?id=7119
Since Node.js reports 0 for start line and column, V8 has no way to adjust the source offsets reported for coverage.

The better way to do it might be to use ``v8::ScriptCompiler::CompileFunctionInContext`, where we ask V8 to wrap the source in a function context. See examples here.

cc @schuay @bcoe

/cc @nodejs/v8

@hashseed

@hashseed is it safe to assume that V8 function would allow us to continue to inject all the same lexically scoped variables?

Would there be a way to call this function via a macro in js land to avoid having to juggle things between the layers? Can you imagine any reason this would cause execution to be different? My biggest concern right now would be how this would affect current ecosystem tools that might be doing magic to account for the wrapper

My gut is that this may be semver major, and potentially break some stuff in the ecosystem... If my gut is wrong this could potentially be semver patch, which would be quite exciting

The better way to do it might be to use ``v8::ScriptCompiler::CompileFunctionInContext`, where we ask V8 to wrap the source in a function context.

I don't remember the details but it didn't quite work last time I tried it: #9273 (comment)

@MylesBorins

Hehe... yeah now that I looked closer v8::ScriptCompiler::CompileFunctionInContext suffers from the same issue. It also just wraps the source at some point.

Looks like I'll have to fix this in V8 first.

@MylesBorins my hope is that it would semantically not break anything.

We need to be careful to not break existing code - returning in a module for instance. My gut feeling is that this breaks a lot of code in the wild doing something like:

if(!process.env.SOME_CONFIG) {
    module.exports = "bar";
    return;
}

causeEffects();
module.exports = "foo";

i feel like we should treat that like we treat

});
some code
(function() {

and just consider it unsupported behavior, as users shouldn't depend on implementation details

Breaking return in a commonjs module context will cause significant havoc.

A V8 api that preserves current behavior without an iife wrap would be nice, though. What about using new Function()? Does that have the same problems?

wouldn't new Function() just become function anonymous(some args) { some body } and have the same problem?

From what I understand the suggested CompileNewFunctionInContext would still be a function and would work - I was pointing out return in case different solutions are suggested.

(idea: would dropping the \n in Module.wrapper fix the line numbering?)

@devsnek

@devsnek I just tested, and yes, it does. What's worse, the offset may change between V8 versions, unlike the known iife wrapper Node uses.

@benjamingr I think the same problem applies to CompileNewFunctionInContext. It still adds wrapper code, but then it's wrapper code we can't safely account for.

My suggestion is to leave the CommonJS system frozen, and instead focus on making things right with the es-module system. Whatever shortcomings Node.js's CommonJS implementation has (and it has several), they haven't prevented a vibrant ecosystem from using them to build things, and fixing those shortcomings is almost certainly going to break that ecosystem in profound and hard-to-predict ways.

(idea: would dropping the \n in Module.wrapper fix the line numbering?)

Only in cases where the error happens on the last line of the script because something wasn't terminated. For example:

$ cat end.js
if ('foo') {

$ node end.js
/Users/isaacs/dev/js/ayo/end.js:3
});
 ^

SyntaxError: Unexpected token )

But otherwise, the only issue is the column offset on line 1.

@isaacs

⚠️ From a user-land view I want to be very careful about changes to Node CJS wrapping. If there is an API change (should try to avoid) it should still be user-land accessible since it does aid in dev related packages. ⚠️

Update:

Just noticed @isaacs' comment:

My suggestion is to leave the CommonJS system frozen, and instead focus on making things right with the es-module system. Whatever shortcomings Node.js's CommonJS implementation has (and it has several), they haven't prevented a vibrant ecosystem from using them to build things, and fixing those shortcomings is almost certainly going to break that ecosystem in profound and hard-to-predict ways.

💯 ++

+1 to freezing commonjs system, anything to keep people moving to esm is good

@jdalton

@jdalton @isaacs @hashseed locking the existing approach for CJS seems workable, as long as the new ESM system provides appropriate context in the scriptParsed event of the debugger. Here's what the meta information looks like currently when you parse a module in v8:

{
  "method": "Debugger.scriptParsed",
  "params": {
    "scriptId": "72",
    "url": "/Users/benjamincoe/bcoe/c8/test/fixtures/b.js",
    "startLine": 0,
    "startColumn": 0,
    "endLine": 12,
    "endColumn": 3,
    "executionContextId": 1,
    "hash": "1A47AD409B904C96B5BC49D6A58187D4B2D1C2C4",
    "isLiveEdit": false,
    "sourceMapURL": "",
    "hasSourceURL": false,
    "isModule": false,
    "length": 171,
    "stackTrace": {
      "callFrames": [
        {
          "functionName": "createScript",
          "scriptId": "41",
          "url": "vm.js",
          "lineNumber": 79,
          "columnNumber": 9
        }
      ]
    }
  }
}

My expectation would be that anything that comes through the new module system would have isModule = true and would not have wonky offsets?

Another thing that I think would be nice would be to expose the fact that we have a 62 byte prefix in a variable, e.g., require.cjsPrefix? This would allow tooling to avoid the 62 byte magic #.

module: revert module._compile to original state if module is patched