refactor(runtime): drop the require() optional-module placeholder

NathanWalker · NathanWalker · commit ff19da99fc6e · 2026-08-12T14:55:29.000-07:00
diff --git a/NativeScript/runtime/ModuleInternal.h b/NativeScript/runtime/ModuleInternal.h
@@ -46,9 +46,6 @@ class ModuleInternal {
                           const std::string& moduleName);
   std::string ResolvePathFromPackageJson(const std::string& packageJson,
                                          bool& error);
-  v8::Local<v8::Object> CreatePlaceholderModule(v8::Isolate* isolate,
-                                                const std::string& moduleName,
-                                                const std::string& cacheKey);
   static v8::ScriptCompiler::CachedData* LoadScriptCache(
       const std::string& path);
   static void SaveScriptCache(const v8::Local<v8::Script> script,
diff --git a/NativeScript/runtime/ModuleInternal.mm b/NativeScript/runtime/ModuleInternal.mm
@@ -22,46 +22,6 @@
 
 namespace tns {
 
-// require()-path policy only: import() rejects a missing bare specifier outright
-// (ESM optionality is `try { await import(x) } catch {}` at the call site, and in
-// dev sessions a bare specifier the import map doesn't cover is a config bug that
-// must fail loudly — see docs/knowledge/hmr-simplification-pass.md §2).
-static bool IsLikelyOptionalModule(const std::string& moduleName) {
-  // Node built-ins are handled by their own dedicated resolution path; never treat them as
-  // an optional external module.
-  if (moduleName.rfind("node:", 0) == 0) {
-    return false;
-  }
-
-  // Check if it's a bare module name (no path separators) that could be an npm package.
-  //
-  // Bare specifiers that end in a recognizable script/data extension (e.g. "foo.js",
-  // "config.json") are explicit file references, not npm-style package names — real npm
-  // package names don't carry a file extension. Treating them as "likely optional" would
-  // swallow a genuine "module not found" failure behind a lazily-throwing placeholder
-  // instead of letting require()/import() fail immediately, which is what callers (and the
-  // existing "should throw error if cant find node module" test) expect for those names.
-  //
-  // This carve-out is deliberately narrow: a dotted bare name that doesn't end in one of
-  // these exact extensions (e.g. "lodash.debounce") is still treated as optional, same as
-  // before. See ModuleInternal.mm/ModuleInternalCallbacks.mm optional-module tests for the
-  // cases this boundary is expected to hold for.
-  static const char* kExplicitFileExtensions[] = {".js", ".mjs", ".cjs", ".json", ".node", ".ts"};
-
-  if (moduleName.find('/') == std::string::npos && moduleName.find('\\') == std::string::npos &&
-      moduleName[0] != '.' && moduleName[0] != '~' && moduleName[0] != '/') {
-    for (const char* ext : kExplicitFileExtensions) {
-      size_t extLen = strlen(ext);
-      if (moduleName.size() > extLen &&
-          moduleName.compare(moduleName.size() - extLen, extLen, ext) == 0) {
-        return false;
-      }
-    }
-    return true;
-  }
-  return false;
-}
-
 // Helper function to check if a file path is an ES module (.mjs) but not a source map (.mjs.map)
 bool IsESModule(const std::string& path) {
   return path.size() >= 4 && path.compare(path.size() - 4, 4, ".mjs") == 0 &&
@@ -624,14 +584,6 @@ static inline void SetOutErrorMessage(std::string* outErrorMessage, const std::s
   }
 
   if (path.empty()) {
-    // A bare specifier shaped like an npm package name resolves to a
-    // lazily-throwing placeholder, so an app can ship without an optional
-    // dependency installed and only fail if it actually touches it. A
-    // specifier resolved against "/" is an explicit absolute path, never an
-    // optional package, so it always hard-fails.
-    if (baseDir != "/" && IsLikelyOptionalModule(moduleName)) {
-      return this->CreatePlaceholderModule(isolate, moduleName, cacheKey);
-    }
     throw NativeScriptException(isolate, "Cannot find module '" + moduleName + "'", "Error");
   }
 
@@ -1615,12 +1567,6 @@ throw NativeScriptException(
   }
 
   if (exists == NO) {
-    // No path for an optional-looking package: LoadImpl turns the empty result
-    // into the placeholder module rather than a hard failure.
-    if (IsLikelyOptionalModule(moduleName)) {
-      return std::string();
-    }
-
     // Create a detailed error message with context
     std::string errorMsg = "Cannot find module '" + moduleName + "'";
     errorMsg += "\n  Base directory: " + baseDir;
@@ -1724,94 +1670,6 @@ throw NativeScriptException(
   return std::string([[basePath stringByAppendingPathExtension:@"js"] UTF8String]);
 }
 
-Local<Object> ModuleInternal::CreatePlaceholderModule(Isolate* isolate,
-                                                      const std::string& moduleName,
-                                                      const std::string& cacheKey) {
-  Local<Context> context = isolate->GetCurrentContext();
-
-  // Create a module object with exports that throws when accessed
-  Local<Object> moduleObj = Object::New(isolate);
-
-  // Create a Proxy that throws an error when any property is accessed.
-  //
-  // The message is passed to a constant factory script as a real V8 string
-  // (never interpolated into the script source). Interpolation is a JS
-  // injection hazard: the message itself contains single quotes
-  // ("Module 'zip' is not available...") which terminated the string literal
-  // early and made the generated script a guaranteed SyntaxError
-  // ("missing ) after argument list") that propagated out of require() with
-  // no hint of its origin.
-  std::string errorMessage =
-      "Module '" + moduleName + "' is not available. This is an optional module.";
-  static const char* kProxyFactorySource = "(function(msg) {"
-                                           "  const error = new Error(msg);"
-                                           "  return new Proxy({}, {"
-                                           "    get: function(target, prop) {"
-                                           "      throw error;"
-                                           "    },"
-                                           "    set: function(target, prop, value) {"
-                                           "      throw error;"
-                                           "    },"
-                                           "    has: function(target, prop) {"
-                                           "      return false;"
-                                           "    },"
-                                           "    ownKeys: function(target) {"
-                                           "      return [];"
-                                           "    },"
-                                           "    getPrototypeOf: function(target) {"
-                                           "      return null;"
-                                           "    }"
-                                           "  });"
-                                           "})";
-
-  TryCatch tc(isolate);
-  Local<Script> proxyScript;
-  if (Script::Compile(context, tns::ToV8String(isolate, kProxyFactorySource))
-          .ToLocal(&proxyScript)) {
-    Local<Value> factoryValue;
-    if (proxyScript->Run(context).ToLocal(&factoryValue) && factoryValue->IsFunction()) {
-      Local<Value> args[]{tns::ToV8String(isolate, errorMessage.c_str())};
-      Local<Value> proxyObject;
-      if (factoryValue.As<v8::Function>()
-              ->Call(context, v8::Undefined(isolate), 1, args)
-              .ToLocal(&proxyObject)) {
-        // Set the exports to the proxy object
-        bool success = moduleObj->Set(context, tns::ToV8String(isolate, "exports"), proxyObject)
-                           .FromMaybe(false);
-        if (!success) {
-          Log(@"Warning: Failed to set exports property on proxy module object");
-        }
-      }
-    }
-  }
-  if (tc.HasCaught()) {
-    // The placeholder is best-effort. Never let its construction machinery
-    // leak an exception that masks the real "module not found" condition.
-    Log(@"Warning: placeholder module construction failed for %s", moduleName.c_str());
-  }
-
-  // Set up the module object
-  bool success = moduleObj
-                     ->Set(context, tns::ToV8String(isolate, "id"),
-                           tns::ToV8String(isolate, moduleName.c_str()))
-                     .FromMaybe(false);
-  if (!success) {
-    Log(@"Warning: Failed to set id property on module object");
-  }
-
-  success =
-      moduleObj->Set(context, tns::ToV8String(isolate, "loaded"), v8::Boolean::New(isolate, true))
-          .FromMaybe(false);
-  if (!success) {
-    Log(@"Warning: Failed to set loaded property on module object");
-  }
-
-  // Cache the placeholder module
-  this->loadedModules_[cacheKey] = std::make_shared<Persistent<Object>>(isolate, moduleObj);
-
-  return moduleObj;
-}
-
 ScriptCompiler::CachedData* ModuleInternal::LoadScriptCache(const std::string& path) {
   std::string canonicalPath = NormalizePath(path);
   if (RuntimeConfig.IsDebug) {
diff --git a/TestRunner/app/tests/NodeBuiltinsAndOptionalModulesTests.mjs b/TestRunner/app/tests/NodeBuiltinsAndOptionalModulesTests.mjs
@@ -17,11 +17,8 @@ describe("Node built-in and optional module resolution", function () {
     expect(u.protocol).toBe("file:");
   });
 
-  // The optional-module placeholder is require()-path policy only (its specs live in
-  // shared/Require/index.js, pending review — docs/knowledge/optional-module-placeholder.md).
-  // import() deliberately diverges: a missing bare specifier rejects, package-shaped or not,
-  // so a dev-session import-map miss fails loudly instead of resolving to a lazily-throwing
-  // proxy. ESM callers that want optionality can `try { await import(x) } catch {}`.
+  // Missing bare specifiers fail at the request site on both require() and
+  // import(). ESM callers that want optionality can `try { await import(x) } catch {}`.
   it("rejects a missing bare specifier instead of resolving a placeholder", async function () {
     const names = [
       "__ns_optional_test_module__",
@@ -39,6 +36,15 @@ describe("Node built-in and optional module resolution", function () {
       }
       expect(error).not.toBe(null);
       expect(String(error)).toContain("Cannot find module");
+
+      let requireError = null;
+      try {
+        globalThis.require(name);
+      } catch (e) {
+        requireError = e;
+      }
+      expect(requireError).not.toBe(null);
+      expect(String(requireError)).toContain("Cannot find module");
     }
   });
 
diff --git a/TestRunner/app/tests/NsUtilTests.js b/TestRunner/app/tests/NsUtilTests.js
@@ -35,23 +35,15 @@ describe("ns:util", function () {
 
     it("leaves bare specifiers to npm resolution", function () {
         // Bare "util" must never resolve to the ns:util builtin. In this app
-        // no npm "util" package exists, so the loader yields an optional-module
-        // placeholder whose members throw on access — both the require and the
-        // member read need guarding.
-        var resolved;
-        try {
-            resolved = require("util");
-        } catch (e) {
-            resolved = undefined;
-        }
-        expect(resolved).not.toBe(util);
-        var resolvedFormat;
+        // no npm "util" package exists, so require("util") throws.
+        var error;
         try {
-            resolvedFormat = resolved && resolved.format;
+            require("util");
         } catch (e) {
-            resolvedFormat = undefined;
+            error = e;
         }
-        expect(resolvedFormat).not.toBe(util.format);
+        expect(error).toBeDefined();
+        expect(String(error)).toContain("Cannot find module");
     });
 
     describe("format", function () {
