sea: allow requiring core modules with the "node:" prefix by RaisinTen · Pull Request #47779 · nodejs/node · GitHub

Previously, the require() function exposed to the embedded SEA code was calling the internal require() function if the module name belonged to the list of public core modules but the internal require() function does not support loading modules with the "node:" prefix, so this change forwards the calls to another require() function that supports this.

Fixes: nodejs/single-executable#69

bootstrap: fix bug in the snapshot require function and share the code with the SEA require

Previously, requiring node:test while building a snapshot
was failing with a Error: Cannot find module 'node:test'. error, so
this change fixes that and uses the same code path for normalizing ids
for both the snapshot require function as well as the SEA require
function.

Details

Previously:

$ node --snapshot-blob snapshot.blob --build-snapshot <(echo "require('node:test')")
node:internal/main/mksnapshot:105
    throw err;
    ^

Error: Cannot find module 'node:test'.
    at requireForUserSnapshot (node:internal/main/mksnapshot:101:17)
    at /dev/fd/13:1:1
    at main (node:internal/main/mksnapshot:168:5)
    at node:internal/main/mksnapshot:185:1

$ node --snapshot-blob snapshot.blob --build-snapshot <(echo "require('test')")
(node:5389) Warning: built-in module test is not yet supported in user snapshots
(Use `node --trace-warnings ...` to show where the warning was created)
Unknown external reference 0x102d226e0.
<unresolved>
[1]    5389 trace trap  node --snapshot-blob snapshot.blob --build-snapshot <(echo "require('test')")

Now:

$ ./node --snapshot-blob snapshot.blob --build-snapshot <(echo "require('node:test')")
(node:5394) Warning: built-in module node:test is not yet supported in user snapshots
(Use `node --trace-warnings ...` to show where the warning was created)
Unknown external reference 0x105cb25e0.
<unresolved>
[1]    5394 trace trap  ./node --snapshot-blob snapshot.blob --build-snapshot

$ ./node --snapshot-blob snapshot.blob --build-snapshot <(echo "require('test')")
node:internal/main/mksnapshot:106
    throw err;
    ^

Error: Cannot find module 'test'.
    at requireForUserSnapshot (node:internal/main/mksnapshot:102:17)
    at /dev/fd/13:1:1
    at main (node:internal/main/mksnapshot:169:5)
    at node:internal/main/mksnapshot:186:1

Since the Unknown external reference error, which happens now for
the node:test module, was already present for the test module,
it should be okay to fix in a separate PR.

More info about the crash in #47832.

Refs: #47779 (comment)
Signed-off-by: Darshan Sen raisinten@gmail.com

cc @nodejs/single-executable

Previously, the `require()` function exposed to the embedded SEA code was calling the internal `require()` function if the module name belonged to the list of public core modules but the internal `require()` function does not support loading modules with the "node:" prefix, so this change forwards the calls to another `require()` function that supports this. Fixes: nodejs/single-executable#69 Signed-off-by: Darshan Sen <raisinten@gmail.com>

I think we should update the documentation to remove the suggestion of Module.createRequire() now that we create one for them?

LGTM

I think we should update the documentation to remove the suggestion of Module.createRequire() now that we create one for them?

@joyeecheung with this change, the require() function that's exposed to the embedded SEA code is still not fs-based because of the Module.isBuiltin check, so if users need access to the file system, they would still need to use Module.createRequire() to build another require() function.

…e with the SEA require Previously, this code: ```sh node --snapshot-blob snapshot.blob --build-snapshot <(echo "require('node:test')")` ``` was failing with a "Error: Cannot find module 'node:test'." error, so this change fixes that and uses the same code path for normalizing ids for both the snapshot require function as well as the SEA require function. However, building a snapshot when `node:test` is required now fails with: ```sh ./node --snapshot-blob snapshot.blob --build-snapshot <(echo "require('node:test')") (node:5362) Warning: built-in module node:test is not yet supported in user snapshots (Use `node --trace-warnings ...` to show where the warning was created) Unknown external reference 0x10201dd70. <unresolved> [1] 5362 trace trap ./node --snapshot-blob snapshot.blob --build-snapshot ``` which could be solved in a separate PR. Refs: nodejs#47779 (comment) Signed-off-by: Darshan Sen <raisinten@gmail.com>

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

Landed in c868aad

`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 <raisinten@gmail.com>

`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 <raisinten@gmail.com> PR-URL: #47896 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>

Previously, the `require()` function exposed to the embedded SEA code was calling the internal `require()` function if the module name belonged to the list of public core modules but the internal `require()` function does not support loading modules with the "node:" prefix, so this change forwards the calls to another `require()` function that supports this. Fixes: nodejs/single-executable#69 Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: #47779 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

`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 <raisinten@gmail.com> PR-URL: #47896 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>

@RaisinTen any chance you would have time to backport this to v18.x?

Sure, done - #49648

Previously, the `require()` function exposed to the embedded SEA code was calling the internal `require()` function if the module name belonged to the list of public core modules but the internal `require()` function does not support loading modules with the "node:" prefix, so this change forwards the calls to another `require()` function that supports this. Fixes: nodejs/single-executable#69 Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: #47779 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Signed-off-by: Darshan Sen <raisinten@gmail.com>

`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 <raisinten@gmail.com> PR-URL: #47896 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>

Previously, the `require()` function exposed to the embedded SEA code was calling the internal `require()` function if the module name belonged to the list of public core modules but the internal `require()` function does not support loading modules with the "node:" prefix, so this change forwards the calls to another `require()` function that supports this. Fixes: nodejs/single-executable#69 Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: nodejs/node#47779 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Signed-off-by: Darshan Sen <raisinten@gmail.com>

`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>

Previously, the `require()` function exposed to the embedded SEA code was calling the internal `require()` function if the module name belonged to the list of public core modules but the internal `require()` function does not support loading modules with the "node:" prefix, so this change forwards the calls to another `require()` function that supports this. Fixes: nodejs/single-executable#69 Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: nodejs/node#47779 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Signed-off-by: Darshan Sen <raisinten@gmail.com>

`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>

Previously, the `require()` function exposed to the embedded SEA code was calling the internal `require()` function if the module name belonged to the list of public core modules but the internal `require()` function does not support loading modules with the "node:" prefix, so this change forwards the calls to another `require()` function that supports this. Fixes: nodejs/single-executable#69 Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: nodejs/node#47779 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Signed-off-by: Darshan Sen <raisinten@gmail.com>

RaisinTen added the single-executable Issues and PRs related to single-executable applications label Apr 29, 2023

nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Apr 29, 2023

RaisinTen mentioned this pull request Apr 29, 2023

Missing internal module nodejs/single-executable#69

Closed

This comment was marked as outdated.

Sign in to view

ljharb approved these changes Apr 29, 2023

View reviewed changes

cjihrig approved these changes Apr 29, 2023

View reviewed changes

RaisinTen added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 29, 2023

lpinca approved these changes Apr 29, 2023

View reviewed changes

github-actions Bot mentioned this pull request Apr 30, 2023

CI Reliability 2023-04-30 nodejs/reliability#551

Open

33 tasks

This comment was marked as outdated.

Sign in to view

joyeecheung reviewed Apr 30, 2023

View reviewed changes

github-actions Bot mentioned this pull request May 1, 2023

CI Reliability 2023-05-01 nodejs/reliability#552

Open

33 tasks

mhdawson approved these changes May 1, 2023

View reviewed changes

joyeecheung reviewed May 1, 2023

View reviewed changes

Comment thread lib/internal/util/embedding.js Outdated

This was referenced May 2, 2023

CI Reliability 2023-05-02 nodejs/reliability#553

Open

CI Reliability 2023-05-03 nodejs/reliability#554

Open

aymen94 approved these changes May 3, 2023

View reviewed changes

RaisinTen mentioned this pull request May 3, 2023

Support snapshotting node:test #47832

Closed

tony-go approved these changes May 3, 2023

View reviewed changes

Comment thread lib/internal/bootstrap/realm.js

github-actions Bot mentioned this pull request May 4, 2023

CI Reliability 2023-05-04 nodejs/reliability#555

Open

28 tasks

joyeecheung approved these changes May 4, 2023

View reviewed changes

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 4, 2023

nodejs-github-bot merged commit c868aad into nodejs:main May 4, 2023

RaisinTen deleted the sea-allow-requiring-core-modules-with-node-prefix branch May 4, 2023 14:33

github-actions Bot mentioned this pull request May 5, 2023

CI Reliability 2023-05-05 nodejs/reliability#556

Open

22 tasks

RaisinTen mentioned this pull request May 6, 2023

module: refactor to use normalizeRequirableId in the CJS module loader #47896

Merged

targos mentioned this pull request May 15, 2023

v20.2.0 release proposal #48020

Merged

danielleadams added the backport-requested-v18.x label Jul 5, 2023

RaisinTen mentioned this pull request Sep 14, 2023

[v18.x backport] sea: allow requiring core modules with the "node:" prefix #49648

Closed

RaisinTen added backport-open-v18.x and removed backport-requested-v18.x labels Sep 14, 2023

RaisinTen added backported-to-v18.x and removed backport-open-v18.x labels Nov 10, 2023

targos mentioned this pull request Nov 28, 2023

v18.19.0 release proposal #50953

Merged

sea: allow requiring core modules with the "node:" prefix by RaisinTen · Pull Request #47779 · nodejs/node · GitHub

Uh oh!

Uh oh!

sea: allow requiring core modules with the "node:" prefix - #47779

sea: allow requiring core modules with the "node:" prefix#47779
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
RaisinTen:sea-allow-requiring-core-modules-with-node-prefix

RaisinTen commented Apr 29, 2023 •
edited
Loading

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

joyeecheung left a comment

Uh oh!

mhdawson left a comment

Uh oh!

RaisinTen commented May 1, 2023

Uh oh!

Uh oh!

nodejs-github-bot commented May 3, 2023

Uh oh!

Uh oh!

nodejs-github-bot commented May 4, 2023

Uh oh!

aduh95 commented Sep 13, 2023

Uh oh!

RaisinTen commented Sep 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

sea: allow requiring core modules with the "node:" prefix by RaisinTen · Pull Request #47779 · nodejs/node · GitHub

Uh oh!

Uh oh!

sea: allow requiring core modules with the "node:" prefix - #47779

sea: allow requiring core modules with the "node:" prefix#47779nodejs-github-bot merged 2 commits intonodejs:mainnodejs/node:mainfrom RaisinTen:sea-allow-requiring-core-modules-with-node-prefixRaisinTen/node:sea-allow-requiring-core-modules-with-node-prefixCopy head branch name to clipboard

Conversation

RaisinTen commented Apr 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

RaisinTen commented May 1, 2023

Uh oh!

Uh oh!

nodejs-github-bot commented May 3, 2023

Uh oh!

Uh oh!

nodejs-github-bot commented May 4, 2023

Uh oh!

aduh95 commented Sep 13, 2023

Uh oh!

RaisinTen commented Sep 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

sea: allow requiring core modules with the "node:" prefix#47779
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
RaisinTen:sea-allow-requiring-core-modules-with-node-prefix

RaisinTen commented Apr 29, 2023 •
edited
Loading