module: refactor to use normalizeRequirableId in the CJS module loader

`BuiltinModule.normalizeRequirableId()` was introduced in #47779 to fix a bug in the require function of SEAs and Snapshots, so that built-in modules with the `node:` scheme could be required correctly. This change makes more use of this API instead of `BuiltinModule.canBeRequiredByUsers()` and `BuiltinModule.canBeRequiredWithoutScheme()` to reduce chances of such bugs. Signed-off-by: Darshan Sen <[email protected]> PR-URL: #47896 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
nodejs · Nov 10, 2023 · 6f0458d · 6f0458d
1 parent 12d731e
commit 6f0458d
Show file tree

Hide file tree

Showing 3 changed files with 13 additions and 26 deletions.
diff --git a/lib/internal/bootstrap/realm.js b/lib/internal/bootstrap/realm.js
@@ -300,17 +300,16 @@ class BuiltinModule {
   }
 
   static normalizeRequirableId(id) {
-    let normalizedId = id;
     if (StringPrototypeStartsWith(id, 'node:')) {
-      normalizedId = StringPrototypeSlice(id, 5);
-    }
-
-    if (!BuiltinModule.canBeRequiredByUsers(normalizedId) ||
-        (id === normalizedId && !BuiltinModule.canBeRequiredWithoutScheme(normalizedId))) {
-      return undefined;
+      const normalizedId = StringPrototypeSlice(id, 5);
+      if (BuiltinModule.canBeRequiredByUsers(normalizedId)) {
+        return normalizedId;
+      }
+    } else if (BuiltinModule.canBeRequiredWithoutScheme(id)) {
+      return id;
     }
 
-    return normalizedId;
+    return undefined;
   }
 
   static getSchemeOnlyModuleNames() {

diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -782,12 +782,7 @@ if (isWindows) {
 }
 
 Module._resolveLookupPaths = function(request, parent) {
-  if ((
-    StringPrototypeStartsWith(request, 'node:') &&
-    BuiltinModule.canBeRequiredByUsers(StringPrototypeSlice(request, 5))
-  ) || (
-    BuiltinModule.canBeRequiredWithoutScheme(request)
-  )) {
+  if (BuiltinModule.normalizeRequirableId(request)) {
     debug('looking for %j in []', request);
     return null;
   }
@@ -979,14 +974,7 @@ Module._load = function(request, parent, isMain) {
 };
 
 Module._resolveFilename = function(request, parent, isMain, options) {
-  if (
-    (
-      StringPrototypeStartsWith(request, 'node:') &&
-      BuiltinModule.canBeRequiredByUsers(StringPrototypeSlice(request, 5))
-    ) || (
-      BuiltinModule.canBeRequiredWithoutScheme(request)
-    )
-  ) {
+  if (BuiltinModule.normalizeRequirableId(request)) {
     return request;
   }
 

diff --git a/test/fixtures/errors/force_colors.snapshot b/test/fixtures/errors/force_colors.snapshot
@@ -4,10 +4,10 @@ throw new Error('Should include grayed stack trace')
 
 Error: Should include grayed stack trace
     at Object.<anonymous> [90m(/[39mtest*force_colors.js:1:7[90m)[39m
-[90m    at Module._compile (node:internal*modules*cjs*loader:1257:14)[39m
-[90m    at Module._extensions..js (node:internal*modules*cjs*loader:1311:10)[39m
-[90m    at Module.load (node:internal*modules*cjs*loader:1115:32)[39m
-[90m    at Module._load (node:internal*modules*cjs*loader:955:12)[39m
+[90m    at Module._compile (node:internal*modules*cjs*loader:1245:14)[39m
+[90m    at Module._extensions..js (node:internal*modules*cjs*loader:1299:10)[39m
+[90m    at Module.load (node:internal*modules*cjs*loader:1103:32)[39m
+[90m    at Module._load (node:internal*modules*cjs*loader:950:12)[39m
 [90m    at Function.executeUserEntryPoint [as runMain] (node:internal*modules*run_main:88:12)[39m
 [90m    at node:internal*main*run_main_module:23:47[39m