Skip to content

Conversation

@RaisinTen
Copy link
Member

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.

`BuiltinModule.normalizeRequirableId()` was introduced in nodejs#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]>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules
  • @nodejs/startup

@nodejs-github-botnodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 6, 2023
@GeoffreyBooth
Copy link
Member

And the ESM loader?

@debadree25debadree25 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 6, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 6, 2023
@nodejs-github-bot
Copy link
Collaborator

@RaisinTen
Copy link
MemberAuthor

@GeoffreyBooth there are still some places both in the ESM loader as well as the CJS loader where canBeRequiredByUsers and canBeRequiredWithoutScheme are being used together, so these could potentially be replaced with normalizeRequirableId:

ESM -

(builtinName)=>{
if(BuiltinModule.canBeRequiredByUsers(builtinName)&&
BuiltinModule.canBeRequiredWithoutScheme(builtinName)){
returnrequire(builtinName);
}
thrownewERR_INVALID_ARG_VALUE('builtinName',builtinName);
},

CJS -

if(StringPrototypeStartsWith(request,'node:')){
// Slice 'node:' prefix
constid=StringPrototypeSlice(request,5);
if(!BuiltinModule.canBeRequiredByUsers(id)){
thrownewERR_UNKNOWN_BUILTIN_MODULE(request);
}
constmodule=loadBuiltinModule(id,request);
returnmodule.exports;
}
constfilename=Module._resolveFilename(request,parent,isMain);
constcachedModule=Module._cache[filename];
if(cachedModule!==undefined){
updateChildren(parent,cachedModule,true);
if(!cachedModule.loaded){
constparseCachedModule=cjsParseCache.get(cachedModule);
if(!parseCachedModule||parseCachedModule.loaded)
returngetExportsForCircularRequire(cachedModule);
parseCachedModule.loaded=true;
}else{
returncachedModule.exports;
}
}
if(BuiltinModule.canBeRequiredWithoutScheme(filename)){
constmod=loadBuiltinModule(filename,request);
returnmod.exports;
}

but I wasn't sure how that could be done, so I kept this PR isolated to only these changes. If you have any suggestions on how that could be done, I'd be happy to implement that.

@RaisinTenRaisinTen added the commit-queue Add this label to land a pull request using GitHub Actions. label May 8, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 8, 2023
@nodejs-github-botnodejs-github-bot merged commit 7fe4745 into nodejs:mainMay 8, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 7fe4745

@RaisinTenRaisinTen deleted the use-BuiltinModule.normalizeRequirableId-where-possible branch May 8, 2023 08:17
targos pushed a commit that referenced this pull request May 12, 2023
`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]>
@targostargos mentioned this pull request May 15, 2023
targos pushed a commit that referenced this pull request Nov 10, 2023
`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]>
@targostargos mentioned this pull request Nov 28, 2023
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
`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 <[email protected]> PR-URL: nodejs/node#47896 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
`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 <[email protected]> PR-URL: nodejs/node#47896 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@RaisinTen@nodejs-github-bot@GeoffreyBooth@anonrig@VoltrexKeyva@danielleadams@debadree25