loader: require.resolve should throw for unknown builtin modules #43336

zhmushan · 2022-06-07T04:12:07Z

Fixes: #43274

nodejs-github-bot · 2022-06-07T04:12:10Z

Review requested:

@nodejs/modules

ljharb

LGTM, thanks for fixing this!

mscdex · 2022-06-07T04:58:06Z

s/unkonw/unknown/ in commit message

zhmushan · 2022-06-07T05:29:47Z

s/unkonw/unknown/ in commit message

Can you explain in detail what this means? I'm not very familiar with the nodejs contribution requirements.
I've added a link to Issue in the latest commit message

DRoet · 2022-06-07T07:01:23Z

@zhmushan

It just means there is a typo in your commit message "unkonw" that should be "unknown".
s/unkonw/unknown/ is just a way to describe a "search and replace".

zhmushan · 2022-06-07T07:12:49Z

It just means there is a typo in your commit message "unkonw" that should be "unknown". s/unkonw/unknown/ is just a way to describe a "search and replace".

Oh, thank you very much. I just realized it was a vim shortcut, forgive me for not knowing anything about it.
I've fixed it.

Fixes: nodejs#43274

zhmushan · 2022-06-07T11:29:44Z

test/parallel/test-require-resolve.js

+    () => require.resolve('node:unknown'),
+    {
+      code: 'MODULE_NOT_FOUND',
+      message: /^Cannot find module 'node:unknown'/,


Quote from the documentation:

If the module can not be found, a MODULE_NOT_FOUND error is thrown.

zhmushan · 2022-06-07T11:34:19Z

lib/internal/modules/cjs/loader.js

@@ -780,19 +780,19 @@ Module._load = function(request, parent, isMain) {
    }
  }

-  const filename = Module._resolveFilename(request, parent, isMain);
-  if (StringPrototypeStartsWith(filename, 'node:')) {


Loading a builtin module prefixed with "node:" expects "ERR_UNKNOWN_BUILTIN_MODULE" to be thrown.
If the "resolve" function is called here, "MODULE_NOT_FOUND" will be thrown.

ERR_UNKNOWN_BUILTIN_MODULE certainly makes more sense here than MODULE_NOT_FOUND, I agree (although I think both are fine).

lib/internal/modules/cjs/loader.js

test/parallel/test-require-resolve.js

Co-authored-by: Antoine du Hamel <[email protected]>

ljharb · 2022-06-08T06:06:04Z

LGTM

nodejs-github-bot · 2022-06-08T08:44:48Z

CI: https://ci.nodejs.org/job/node-test-pull-request/44405/

nodejs-github-bot · 2022-06-12T09:37:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/44465/

nodejs-github-bot · 2022-06-12T11:26:26Z

CI: https://ci.nodejs.org/job/node-test-pull-request/44469/

nodejs-github-bot · 2022-06-13T16:59:06Z

CI: https://ci.nodejs.org/job/node-test-pull-request/44498/

nodejs-github-bot · 2022-06-14T20:08:59Z

Commit Queue failed

- Loading data for nodejs/node/pull/43336
✔  Done loading data for nodejs/node/pull/43336
----------------------------------- PR info ------------------------------------
Title      loader: require.resolve should throw for unknown builtin modules (#43336)
Author     木杉  (@zhmushan)
Branch     zhmushan:require_resolve -> nodejs:master
Labels     module, author ready, needs-ci
Commits    3
 - loader: make require.resolve throw error for unknown builtin modules
 - Update lib/internal/modules/cjs/loader.js
 - Update test/parallel/test-require-resolve.js
Committers 2
 - 木杉 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/43336
Fixes: https://github.com/nodejs/node/issues/43274
Reviewed-By: Antoine du Hamel 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: James M Snell 
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/43336
Fixes: https://github.com/nodejs/node/issues/43274
Reviewed-By: Antoine du Hamel 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: James M Snell 
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 07 Jun 2022 04:12:07 GMT
   ✔  Approvals: 4
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/43336#pullrequestreview-999163419
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/43336#pullrequestreview-1000924532
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/43336#pullrequestreview-1003513209
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/43336#pullrequestreview-1006496775
   ✖  Last GitHub CI failed
   ℹ  Last Full PR CI on 2022-06-13T16:59:06Z: https://ci.nodejs.org/job/node-test-pull-request/44498/
- Querying data for job/node-test-pull-request/44498/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/2497846390

aduh95 · 2022-06-17T08:20:58Z

Landed in 7fd4cf4

ljharb · 2022-06-17T13:17:17Z

Yay! What’s the best way to quickly backport this as far as possible?

Fixes: #43274 PR-URL: #43336 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Fixes: nodejs/node#43274 PR-URL: nodejs/node#43336 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jun 7, 2022

ljharb reviewed Jun 7, 2022

View reviewed changes

zhmushan force-pushed the require_resolve branch from 353c538 to 4698313 Compare June 7, 2022 04:16

ljharb added the module Issues and PRs related to the module subsystem. label Jun 7, 2022

zhmushan force-pushed the require_resolve branch from b72fd44 to 55f45c9 Compare June 7, 2022 05:25

zhmushan force-pushed the require_resolve branch from 55f45c9 to abf07ec Compare June 7, 2022 07:09

JakobJingleheimer changed the title ~~loader: require.resolve should throw for unkonw builtin modules~~ loader: require.resolve should throw for unknown builtin modules Jun 7, 2022

zhmushan force-pushed the require_resolve branch from abf07ec to edc7a8e Compare June 7, 2022 11:19

loader: make require.resolve throw error for unknown builtin modules

21bfcaa

Fixes: nodejs#43274

zhmushan force-pushed the require_resolve branch from edc7a8e to 21bfcaa Compare June 7, 2022 11:27

zhmushan commented Jun 7, 2022

View reviewed changes

aduh95 reviewed Jun 7, 2022

View reviewed changes

lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved

test/parallel/test-require-resolve.js Outdated Show resolved Hide resolved

zhmushan and others added 2 commits June 7, 2022 22:27

Update lib/internal/modules/cjs/loader.js

9bbf815

Co-authored-by: Antoine du Hamel <[email protected]>

Update test/parallel/test-require-resolve.js

c65f329

Co-authored-by: Antoine du Hamel <[email protected]>

zhmushan requested a review from ljharb June 8, 2022 02:43

ljharb removed their request for review June 8, 2022 06:06

aduh95 approved these changes Jun 8, 2022

View reviewed changes

aduh95 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 Jun 8, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 8, 2022

This comment was marked as outdated.

Sign in to view

benjamingr approved these changes Jun 9, 2022

View reviewed changes

jasnell approved these changes Jun 12, 2022

View reviewed changes

github-actions bot mentioned this pull request Jun 13, 2022

CI Reliability 2022-06-13 nodejs/reliability#299

Open

15 tasks

github-actions bot mentioned this pull request Jun 14, 2022

CI Reliability 2022-06-14 nodejs/reliability#300

Open

16 tasks

lpinca approved these changes Jun 14, 2022

View reviewed changes

lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 14, 2022

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jun 14, 2022

github-actions bot mentioned this pull request Jun 15, 2022

CI Reliability 2022-06-15 nodejs/reliability#301

Open

17 tasks

aduh95 merged commit 7fd4cf4 into nodejs:main Jun 17, 2022

richardlau added lts-watch-v14.x labels Jun 17, 2022

zhmushan deleted the require_resolve branch June 30, 2022 16:20

targos mentioned this pull request Jul 12, 2022

v18.6.0 release proposal #43789

Merged

targos removed the lts-watch-v16.x label Jul 31, 2022

targos mentioned this pull request Aug 3, 2022

v16.17.0 release proposal #44098

Merged

danielleadams mentioned this pull request Nov 1, 2022

[v14.x backport] loader: make require.resolve throw for unknown builtin modules #45263

Closed

danielleadams added backport-open-v14.x and removed lts-watch-v14.x labels Nov 1, 2022

targos removed the backport-open-v14.x label May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

loader: require.resolve should throw for unknown builtin modules #43336

loader: require.resolve should throw for unknown builtin modules #43336

zhmushan commented Jun 7, 2022 •

edited by aduh95

Loading

nodejs-github-bot commented Jun 7, 2022

ljharb left a comment

mscdex commented Jun 7, 2022

zhmushan commented Jun 7, 2022

DRoet commented Jun 7, 2022

zhmushan commented Jun 7, 2022

zhmushan Jun 7, 2022

zhmushan Jun 7, 2022

aduh95 Jun 7, 2022

ljharb commented Jun 8, 2022

This comment was marked as outdated.

nodejs-github-bot commented Jun 8, 2022

nodejs-github-bot commented Jun 12, 2022

nodejs-github-bot commented Jun 12, 2022

nodejs-github-bot commented Jun 13, 2022

nodejs-github-bot commented Jun 14, 2022

aduh95 commented Jun 17, 2022

ljharb commented Jun 17, 2022

loader: require.resolve should throw for unknown builtin modules #43336

loader: require.resolve should throw for unknown builtin modules #43336

Conversation

zhmushan commented Jun 7, 2022 • edited by aduh95 Loading

nodejs-github-bot commented Jun 7, 2022

ljharb left a comment

Choose a reason for hiding this comment

mscdex commented Jun 7, 2022

zhmushan commented Jun 7, 2022

DRoet commented Jun 7, 2022

zhmushan commented Jun 7, 2022

zhmushan Jun 7, 2022

Choose a reason for hiding this comment

zhmushan Jun 7, 2022

Choose a reason for hiding this comment

aduh95 Jun 7, 2022

Choose a reason for hiding this comment

ljharb commented Jun 8, 2022

This comment was marked as outdated.

nodejs-github-bot commented Jun 8, 2022

nodejs-github-bot commented Jun 12, 2022

nodejs-github-bot commented Jun 12, 2022

nodejs-github-bot commented Jun 13, 2022

nodejs-github-bot commented Jun 14, 2022

aduh95 commented Jun 17, 2022

ljharb commented Jun 17, 2022

zhmushan commented Jun 7, 2022 •

edited by aduh95

Loading