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

RaisinTen · 2023-04-29T06:43:24Z

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.

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 [email protected]

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 <[email protected]>

joyeecheung

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

mhdawson

LGTM

RaisinTen · 2023-05-01T15:26:49Z

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.

lib/internal/util/embedding.js

…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 <[email protected]>

nodejs-github-bot · 2023-05-03T08:05:22Z

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

lib/internal/bootstrap/realm.js

nodejs-github-bot · 2023-05-04T14:32:37Z

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 <[email protected]>

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

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 <[email protected]> PR-URL: #47779 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

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

aduh95 · 2023-09-13T19:35:30Z

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

RaisinTen · 2023-09-14T09:56:33Z

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 <[email protected]> PR-URL: #47779 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Signed-off-by: Darshan Sen <[email protected]>

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

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 <[email protected]> PR-URL: nodejs/node#47779 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Signed-off-by: Darshan Sen <[email protected]>

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

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 <[email protected]> PR-URL: nodejs/node#47779 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Signed-off-by: Darshan Sen <[email protected]>

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

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 <[email protected]> PR-URL: nodejs/node#47779 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Signed-off-by: Darshan Sen <[email protected]>

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 pending requests for changes, 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

lib/internal/util/embedding.js Outdated Show resolved Hide resolved

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

lib/internal/bootstrap/realm.js Show resolved Hide resolved

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 PRs awaiting manual backport to the v18.x-staging branch. 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 Indicate that the PR has an open backport. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. labels Sep 14, 2023

RaisinTen added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-open-v18.x Indicate that the PR has an open backport. labels Nov 10, 2023

targos mentioned this pull request Nov 28, 2023

v18.19.0 release proposal #50953

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

RaisinTen commented Apr 29, 2023 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

joyeecheung left a comment

mhdawson left a comment

RaisinTen commented May 1, 2023

nodejs-github-bot commented May 3, 2023

nodejs-github-bot commented May 4, 2023

aduh95 commented Sep 13, 2023

RaisinTen commented Sep 14, 2023

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

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

Conversation

RaisinTen commented Apr 29, 2023 • edited Loading

This comment was marked as outdated.

This comment was marked as outdated.

joyeecheung left a comment

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

RaisinTen commented May 1, 2023

nodejs-github-bot commented May 3, 2023

nodejs-github-bot commented May 4, 2023

aduh95 commented Sep 13, 2023

RaisinTen commented Sep 14, 2023

RaisinTen commented Apr 29, 2023 •

edited

Loading