Replace old helper with new

GeoffreyBooth · GeoffreyBooth · commit cb86df839369 · 2023-10-14T15:25:09.000-07:00
diff --git 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
@@ -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
@@ -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
@@ -1206,32 +1206,17 @@ void ContextifyContext::CompileFunction(
       data + cached_data_buf->ByteOffset(), cached_data_buf->ByteLength());
   }
 
-  // Set host_defined_options
   Local<PrimitiveArray> 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<Value>(),    // 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<PrimitiveArray> ContextifyContext::GetHostDefinedOptions(
+    Isolate* isolate, Local<Symbol> id_symbol) {
+  Local<PrimitiveArray> 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<String> code,
+    Local<String> filename,
+    int line_offset,
+    int column_offset,
+    Local<PrimitiveArray> 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<Value>(),  // 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<Object> ContextifyContext::CompileFunctionAndCacheResult(
     Environment* env,
     Local<Context> parsing_context,
@@ -1375,35 +1402,11 @@ void ContextifyContext::ContainsModuleSyntax(
                       filename))
           .As<Symbol>();
 
-  // TODO: Abstract this into a separate function
-  // Set host_defined_options
   Local<PrimitiveArray> 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<Value>(),  // 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<Local<String>> params = {
       String::NewFromUtf8(isolate, "exports").ToLocalChecked(),
diff --git a/src/node_contextify.h b/src/node_contextify.h
@@ -93,6 +93,18 @@ class ContextifyContext : public BaseObject {
       bool produce_cached_data,
       v8::Local<v8::Symbol> id_symbol,
       const errors::TryCatchScope& try_catch);
+  static v8::Local<v8::PrimitiveArray> GetHostDefinedOptions(
+      v8::Isolate* isolate, v8::Local<v8::Symbol> id_symbol);
+  static v8::ScriptCompiler::Source GetCommonJSSourceInstance(
+      v8::Isolate* isolate,
+      v8::Local<v8::String> code,
+      v8::Local<v8::String> filename,
+      int line_offset,
+      int column_offset,
+      v8::Local<v8::PrimitiveArray> host_defined_options,
+      v8::ScriptCompiler::CachedData* cached_data);
+  static v8::ScriptCompiler::CompileOptions GetCompileOptions(
+      const v8::ScriptCompiler::Source& source);
   static void ContainsModuleSyntax(
       const v8::FunctionCallbackInfo<v8::Value>& args);
   static void WeakCallback(
