-
Notifications
You must be signed in to change notification settings - Fork 29k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JS source should not be wrapped. #17396
Comments
/cc @nodejs/v8 |
@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 |
I don't remember the details but it didn't quite work last time I tried it: #9273 (comment) |
Hehe... yeah now that I looked closer 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 - 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 A V8 api that preserves current behavior without an iife wrap would be nice, though. What about using |
wouldn't |
From what I understand the suggested CompileNewFunctionInContext would still be a function and would work - I was pointing out (idea: would dropping the |
@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 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. |
Only in cases where the error happens on the last line of the script because something wasn't terminated. For example:
But otherwise, the only issue is the column offset on line 1. |
Update: Just noticed @isaacs' comment:
💯 ++ |
+1 to freezing commonjs system, anything to keep people moving to esm is good |
@jdalton @isaacs @hashseed locking the existing approach for CJS seems workable, as long as the new ESM system provides appropriate context in the {
"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 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., |
Can be gotten from |
Or
from here. |
It looks like we are setting https://github.com/nodejs/node/blob/master/src/module_wrap.cc#L105 So, I think the only work that would shake out of this is ensuring that ES modules don't have the same wrapper offset issue. |
I guess for the particular use case of code coverage we can get the wrapper header offset directly and subtract it from the coverage offsets. Might still be nice to move towards hiding this implementation detail though, without changing the general semantics of the function scope. |
We should probably check if any existing popular projects are using this: https://github.com/search?l=JavaScript&p=2&q=%22module.wrapper%22+%22require%28.module.%29%22&type=Code&utf8=%E2%9C%93 , a quick look didn't show anything terrible |
Nothing, I am not +/- to it, just interested that no statistics were being brought to the table. None of the uses seemed to be abusing the feature for Ill gotten gains while also being popular enough that we need to replicate such a feature for ESM. |
Okay. FWIW I don't think anyone was suggesting the CJS wrapper APIs be added to ESM, just suggesting they not be removed from CJS if it can be avoided. |
Currently, yes. But we're discussing possible solutions where the source string would remain unmodified and the wrapper would be created only as AST nodes by the parser. That'd avoid any complications around weird line/column offsets, and we'd have better verification that the given script is actually valid JS. |
^ That seems strictly superior to me to what we currently have. I'm +1 on switching over. |
That's pretty cool. It looks like something that could be exposed by |
@jdalton I don't think this would be passed the entire function string, just the body of the function. Which wouldn't be able to be compatible with .wrap/.wrapper since it would return a function instead of a string and would not be accepting the function prefix/suffix. |
Oh, ok never mind then 😢 Exposing it to |
I landed the patch to |
@bnoordhuis the stack trace line/column issue should be fixed now. The current API does not support code caching, but it would still be interesting to see whether it works for require. It should be already be a bit faster than using wrappers, and I have some ideas to make it faster still. |
In reply to concerns above: |
Use vm.compileFunction (which is a binding for v8::CompileFunctionInContext) instead of Module.wrap internally in Module._compile for the cjs loader. Fixes: nodejs#17396
Use vm.compileFunction (which is a binding for v8::CompileFunctionInContext) instead of Module.wrap internally in Module._compile for the cjs loader. Fixes: nodejs#17396
Use vm.compileFunction (which is a binding for v8::CompileFunctionInContext) instead of Module.wrap internally in Module._compile for the cjs loader. Fixes: #17396 PR-URL: #21573 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Use vm.compileFunction (which is a binding for v8::CompileFunctionInContext) instead of Module.wrap internally in Module._compile for the cjs loader. Fixes: #17396 PR-URL: #21573 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: nodejs#21573 Fixes: nodejs#17396 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Use vm.compileFunction (which is a binding for v8::CompileFunctionInContext) instead of Module.wrap internally in Module._compile for the cjs loader. Fixes: #17396 Backport-PR-URL: #27124 PR-URL: #21573 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Use vm.compileFunction (which is a binding for v8::CompileFunctionInContext) instead of Module.wrap internally in Module._compile for the cjs loader. Fixes: #17396 Backport-PR-URL: #27124 PR-URL: #21573 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Use vm.compileFunction (which is a binding for v8::CompileFunctionInContext) instead of Module.wrap internally in Module._compile for the cjs loader. Fixes: #17396 Backport-PR-URL: #27124 PR-URL: #21573 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Use vm.compileFunction (which is a binding for v8::CompileFunctionInContext) instead of Module.wrap internally in Module._compile for the cjs loader. Fixes: nodejs/node#17396 PR-URL: nodejs/node#21573 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
When Node.js executes JavaScript, it internally wraps them into an IIFE:
There are some problems with this:
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
The text was updated successfully, but these errors were encountered: