Skip to content

Commit

Permalink
module: refactor to use normalizeRequirableId in the CJS
Browse files Browse the repository at this point in the history
`BuiltinModule.normalizeRequirableId()` was introduced in
nodejs/node#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 <raisinten@gmail.com>
PR-URL: nodejs/node#47896
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
  • Loading branch information
sercher committed Apr 25, 2024
1 parent 449a0bd commit 197530e
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 26 deletions.
15 changes: 7 additions & 8 deletions graal-nodejs/lib/internal/bootstrap/realm.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
16 changes: 2 additions & 14 deletions graal-nodejs/lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}

Expand Down
8 changes: 4 additions & 4 deletions graal-nodejs/test/fixtures/errors/force_colors.snapshot
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ throw new Error('Should include grayed stack trace')

Error: Should include grayed stack trace
at Object.<anonymous> (/test*force_colors.js:1:7)
 at Module._compile (node:internal*modules*cjs*loader:1257:14)
 at Module._extensions..js (node:internal*modules*cjs*loader:1311:10)
 at Module.load (node:internal*modules*cjs*loader:1115:32)
 at Module._load (node:internal*modules*cjs*loader:955:12)
 at Module._compile (node:internal*modules*cjs*loader:1245:14)
 at Module._extensions..js (node:internal*modules*cjs*loader:1299:10)
 at Module.load (node:internal*modules*cjs*loader:1103:32)
 at Module._load (node:internal*modules*cjs*loader:950:12)
 at Function.executeUserEntryPoint [as runMain] (node:internal*modules*run_main:88:12)
 at node:internal*main*run_main_module:23:47

Expand Down

0 comments on commit 197530e

Please sign in to comment.