From cb86df839369c89437698b0fb6c345c42df89224 Mon Sep 17 00:00:00 2001 From: Geoffrey Booth Date: Fri, 13 Oct 2023 16:04:53 -0700 Subject: [PATCH] Replace old helper with new --- lib/internal/modules/cjs/loader.js | 7 +- lib/internal/modules/esm/translators.js | 10 +-- lib/internal/modules/helpers.js | 23 ----- src/node_contextify.cc | 107 ++++++++++++------------ src/node_contextify.h | 12 +++ 5 files changed, 76 insertions(+), 83 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index b3b438372fe9ed..7c91ba49e2cc7b 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -90,6 +90,7 @@ const { makeContextifyScript, runScriptInThisContext, } = require('internal/vm'); +const { containsModuleSyntax } = internalBinding('contextify'); const assert = require('internal/assert'); const fs = require('fs'); @@ -104,7 +105,6 @@ const { const { getCjsConditions, initializeCjsConditions, - hasEsmSyntax, loadBuiltinModule, makeRequireFunction, normalizeReferrerURL, @@ -1315,7 +1315,7 @@ function wrapSafe(filename, content, cjsModuleInstance, codeCache) { } catch (err) { if (process.mainModule === cjsModuleInstance) { const { enrichCJSError } = require('internal/modules/esm/translators'); - enrichCJSError(err, content); + enrichCJSError(err, content, filename); } throw err; } @@ -1400,10 +1400,11 @@ Module._extensions['.js'] = function(module, filename) { const pkg = packageJsonReader.readPackageScope(filename) || { __proto__: null }; // Function require shouldn't be used in ES modules. if (pkg.data?.type === 'module') { + // This is an error path, because `require` of a `.js` file in a `"type": "module"` scope is not allowed. const parent = moduleParentCache.get(module); const parentPath = parent?.filename; const packageJsonPath = path.resolve(pkg.path, 'package.json'); - const usesEsm = hasEsmSyntax(content); + const usesEsm = containsModuleSyntax(content, filename); const err = new ERR_REQUIRE_ESM(filename, usesEsm, parentPath, packageJsonPath); // Attempt to reconstruct the parent require frame. diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index c6540f49811460..2c626c054a770e 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -30,11 +30,11 @@ function lazyTypes() { return _TYPES = require('internal/util/types'); } +const { containsModuleSyntax } = internalBinding('contextify'); const assert = require('internal/assert'); const { readFileSync } = require('fs'); const { dirname, extname, isAbsolute } = require('path'); const { - hasEsmSyntax, loadBuiltinModule, stripBOM, } = require('internal/modules/helpers'); @@ -166,11 +166,11 @@ translators.set('module', async function moduleStrategy(url, source, isMain) { * Provide a more informative error for CommonJS imports. * @param {Error | any} err * @param {string} [content] Content of the file, if known. - * @param {string} [filename] Useful only if `content` is unknown. + * @param {string} [filename] The filename of the erroring module. */ function enrichCJSError(err, content, filename) { if (err != null && ObjectGetPrototypeOf(err) === SyntaxErrorPrototype && - hasEsmSyntax(content || readFileSync(filename, 'utf-8'))) { + containsModuleSyntax(content, filename ?? '')) { // Emit the warning synchronously because we are in the middle of handling // a SyntaxError that will throw and likely terminate the process before an // asynchronous warning would be emitted. @@ -217,7 +217,7 @@ function loadCJSModule(module, source, url, filename) { importModuleDynamically, // importModuleDynamically ).function; } catch (err) { - enrichCJSError(err, source, url); + enrichCJSError(err, source, filename); throw err; } @@ -344,7 +344,7 @@ translators.set('commonjs', async function commonjsStrategy(url, source, assert(module === CJSModule._cache[filename]); CJSModule._load(filename); } catch (err) { - enrichCJSError(err, source, url); + enrichCJSError(err, source, filename); throw err; } } : loadCJSModule; diff --git a/lib/internal/modules/helpers.js b/lib/internal/modules/helpers.js index 7f2959cc469dc1..6b30a1d8c76d4b 100644 --- a/lib/internal/modules/helpers.js +++ b/lib/internal/modules/helpers.js @@ -3,7 +3,6 @@ const { ArrayPrototypeForEach, ArrayPrototypeJoin, - ArrayPrototypeSome, ObjectDefineProperty, ObjectPrototypeHasOwnProperty, SafeMap, @@ -299,32 +298,10 @@ function normalizeReferrerURL(referrer) { return new URL(referrer).href; } -/** - * For error messages only, check if ESM syntax is in use. - * @param {string} code - */ -function hasEsmSyntax(code) { - debug('Checking for ESM syntax'); - const parser = require('internal/deps/acorn/acorn/dist/acorn').Parser; - let root; - try { - root = parser.parse(code, { sourceType: 'module', ecmaVersion: 'latest' }); - } catch { - return false; - } - - return ArrayPrototypeSome(root.body, (stmt) => - stmt.type === 'ExportDefaultDeclaration' || - stmt.type === 'ExportNamedDeclaration' || - stmt.type === 'ImportDeclaration' || - stmt.type === 'ExportAllDeclaration'); -} - module.exports = { addBuiltinLibsToObject, getCjsConditions, initializeCjsConditions, - hasEsmSyntax, loadBuiltinModule, makeRequireFunction, normalizeReferrerURL, diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 5b1c4c9a99d77e..1aa46fd95ddbd4 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -1206,32 +1206,17 @@ void ContextifyContext::CompileFunction( data + cached_data_buf->ByteOffset(), cached_data_buf->ByteLength()); } - // Set host_defined_options Local host_defined_options = - PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength); - host_defined_options->Set( - isolate, loader::HostDefinedOptions::kID, id_symbol); - - ScriptOrigin origin(isolate, - filename, - line_offset, // line offset - column_offset, // column offset - true, // is cross origin - -1, // script id - Local(), // source map URL - false, // is opaque (?) - false, // is WASM - false, // is ES Module - host_defined_options); - + GetHostDefinedOptions(isolate, id_symbol); ScriptCompiler::Source source = - ScriptCompiler::Source(code, origin, cached_data); - ScriptCompiler::CompileOptions options; - if (source.GetCachedData() == nullptr) { - options = ScriptCompiler::kNoCompileOptions; - } else { - options = ScriptCompiler::kConsumeCodeCache; - } + GetCommonJSSourceInstance(isolate, + code, + filename, + line_offset, + column_offset, + host_defined_options, + cached_data); + ScriptCompiler::CompileOptions options = GetCompileOptions(source); Context::Scope scope(parsing_context); @@ -1279,6 +1264,48 @@ void ContextifyContext::CompileFunction( args.GetReturnValue().Set(result); } +Local ContextifyContext::GetHostDefinedOptions( + Isolate* isolate, Local id_symbol) { + Local host_defined_options = + PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength); + host_defined_options->Set( + isolate, loader::HostDefinedOptions::kID, id_symbol); + return host_defined_options; +} + +ScriptCompiler::Source ContextifyContext::GetCommonJSSourceInstance( + Isolate* isolate, + Local code, + Local filename, + int line_offset, + int column_offset, + Local host_defined_options, + ScriptCompiler::CachedData* cached_data) { + ScriptOrigin origin(isolate, + filename, + line_offset, // line offset + column_offset, // column offset + true, // is cross origin + -1, // script id + Local(), // source map URL + false, // is opaque (?) + false, // is WASM + false, // is ES Module + host_defined_options); + return ScriptCompiler::Source(code, origin, cached_data); +} + +ScriptCompiler::CompileOptions ContextifyContext::GetCompileOptions( + const ScriptCompiler::Source& source) { + ScriptCompiler::CompileOptions options; + if (source.GetCachedData() != nullptr) { + options = ScriptCompiler::kConsumeCodeCache; + } else { + options = ScriptCompiler::kNoCompileOptions; + } + return options; +} + Local ContextifyContext::CompileFunctionAndCacheResult( Environment* env, Local parsing_context, @@ -1375,35 +1402,11 @@ void ContextifyContext::ContainsModuleSyntax( filename)) .As(); - // TODO: Abstract this into a separate function - // Set host_defined_options Local host_defined_options = - PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength); - host_defined_options->Set( - isolate, loader::HostDefinedOptions::kID, id_symbol); - - ScriptOrigin origin(isolate, - filename, - 0, // line offset - 0, // column offset - true, // is cross origin - -1, // script id - Local(), // source map URL - false, // is opaque (?) - false, // is WASM - false, // is ES Module - host_defined_options); - - ScriptCompiler::CachedData* cached_data = nullptr; - ScriptCompiler::Source source = - ScriptCompiler::Source(code, origin, cached_data); - ScriptCompiler::CompileOptions options; - if (source.GetCachedData() == nullptr) { - options = ScriptCompiler::kNoCompileOptions; - } else { - options = ScriptCompiler::kConsumeCodeCache; - } - // End TODO + GetHostDefinedOptions(isolate, id_symbol); + ScriptCompiler::Source source = GetCommonJSSourceInstance( + isolate, code, filename, 0, 0, host_defined_options, nullptr); + ScriptCompiler::CompileOptions options = GetCompileOptions(source); std::vector> params = { String::NewFromUtf8(isolate, "exports").ToLocalChecked(), diff --git a/src/node_contextify.h b/src/node_contextify.h index 9779711e33e440..997e98638005bc 100644 --- a/src/node_contextify.h +++ b/src/node_contextify.h @@ -93,6 +93,18 @@ class ContextifyContext : public BaseObject { bool produce_cached_data, v8::Local id_symbol, const errors::TryCatchScope& try_catch); + static v8::Local GetHostDefinedOptions( + v8::Isolate* isolate, v8::Local id_symbol); + static v8::ScriptCompiler::Source GetCommonJSSourceInstance( + v8::Isolate* isolate, + v8::Local code, + v8::Local filename, + int line_offset, + int column_offset, + v8::Local host_defined_options, + v8::ScriptCompiler::CachedData* cached_data); + static v8::ScriptCompiler::CompileOptions GetCompileOptions( + const v8::ScriptCompiler::Source& source); static void ContainsModuleSyntax( const v8::FunctionCallbackInfo& args); static void WeakCallback(