GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — module: bootstrap module loaders in shadow realm by legendecas · Pull Request #48655 · nodejs/node

Conversation

@legendecas

@legendecas legendecas commented Jul 5, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This bootstraps ESM loaders in the ShadowRealm with ShadowRealm.prototype.importValue as its entry point and enables loading ESM and CJS modules in the ShadowRealm. The module is imported without a parent URL and resolved with the current process's working directory.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss, lin72h, and mcollina reacted with thumbs up emoji
All reactions
  • 👍 3 reactions
@nodejs-github-bot

nodejs-github-bot commented Jul 5, 2023 •
edited by legendecas
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules
  • @nodejs/realm
  • @nodejs/startup
  • @nodejs/v8-update
  • @nodejs/vm
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Jul 5, 2023
@legendecas legendecas added module Issues and PRs related to the module subsystem. realm Issues and PRs related to the ShadowRealm API and node::Realm. labels Jul 5, 2023
ljharb
ljharb reviewed Jul 5, 2023
View reviewed changes
Comment thread lib/internal/bootstrap/realm.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/internal/vm/module.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@legendecas
legendecas force-pushed the shadow-realm/module branch from e1e63e3 to 23a9cb4 Compare August 15, 2023 16:38
@legendecas

legendecas commented Aug 15, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Rebased on the tip of the main branch and removed dependencies on #48510 (with known limitation on memory leaks). Marking as ready to be reviewed. This PR would unblock the verification of built-in modules support in the shadow realm.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@legendecas
legendecas marked this pull request as ready for review August 15, 2023 17:52
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 15, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 15, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 15, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

joyeecheung
joyeecheung reviewed Aug 15, 2023
View reviewed changes
Comment thread lib/internal/bootstrap/shadow_realm.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@mcollina mcollina closed this Aug 16, 2023
@mcollina mcollina reopened this Aug 16, 2023
mcollina
mcollina approved these changes Aug 16, 2023
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@legendecas
legendecas force-pushed the shadow-realm/module branch from 23a9cb4 to f1f3986 Compare August 17, 2023 15:44
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 17, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 17, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 17, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@GeoffreyBooth

GeoffreyBooth commented Aug 17, 2023

Copy link
Copy Markdown
Member

We’re working on eventually removing the CommonJS loader, leaving only the ESM loader as the single module loader (as it can load both CommonJS and ESM code). Is it possible to refactor this so that ShadowRealm always loads everything only through the ESM loader? Then this part of things won’t need refactoring when the CommonJS loader goes away, and there would also be more uniformity in how modules are loaded within ShadowRealm.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@legendecas

legendecas commented Aug 17, 2023

Copy link
Copy Markdown
Member Author

@GeoffreyBooth The ShadowRealm interface only exposes ESM ShadowRealm.prototype.importValue(speficier) and no CJS equivalent. The test case in the PR merely shows that the modules imported in the ShadowRealm can create a require function, just like what we can do in the principal realm.

Are you suggesting this PR didn't load modules in ShadowRealm.prototype.importValue(speficier) with the ESM loader? If so, could you share a hint of how I can fix that?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@GeoffreyBooth

GeoffreyBooth commented Aug 17, 2023

Copy link
Copy Markdown
Member

Are you suggesting this PR didn’t load modules in ShadowRealm.prototype.importValue(speficier) with the ESM loader? If so, could you share a hint of how I can fix that?

I was just responding to the PR description, “This bootstraps ESM and CJS module loaders in the ShadowRealm.” Did what you mean to say that it allows for loading ESM and CJS modules in the ShadowRealm?

I don’t really follow what a lot of the code in this PR does. If its only internal interactions with module loaders are through the ESM loader, then we’re good.

legendecas reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@legendecas

legendecas commented Aug 17, 2023

Copy link
Copy Markdown
Member Author

Yes, CJS support in ShadowRealm is enabled by ESM loaders. It seems that the PR description is misleading somehow, I've fixed the wording.

GeoffreyBooth reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@GeoffreyBooth

GeoffreyBooth commented Aug 17, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

The module is imported without a parent URL and resolved with the current process’s working directory.

We had a similar situation with node:module register, of needing to resolve relative to process.cwd as an unfortunate default that we dislike but couldn’t find a better alternative for. We added a second parameter to register allowing the user to pass parentURL to resolve against, so that most usage would be like register('ts-node', import.meta.url). You might want to consider the same here.

Also please make sure that the string is interpreted as a file URL and not as a path, because import specifiers must be file URLs and you’re calling this API importValue. Then users can do things like realm.importValue(import.meta.resolve('./file.js'), 'foo').

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

GeoffreyBooth
GeoffreyBooth reviewed Aug 17, 2023
View reviewed changes
Comment thread test/parallel/test-shadow-realm-import-value-resolve.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Aug 18, 2023
CI Reliability 2023-08-18 nodejs/reliability#640
Open
CI Reliability 2023-08-21 nodejs/reliability#641
Open
@legendecas legendecas mentioned this pull request Aug 21, 2023
test: verify dynamic import call with absolute path strings #49275
Merged
@github-actions github-actions Bot mentioned this pull request Aug 22, 2023
CI Reliability 2023-08-22 nodejs/reliability#642
Open
21 tasks
legendecas added a commit that referenced this pull request Nov 13, 2023
@legendecas
module: bootstrap module loaders in shadow realm
fc2862b
This bootstraps ESM loaders in the ShadowRealm with
`ShadowRealm.prototype.importValue` as its entry point and enables
loading ESM and CJS modules in the ShadowRealm. The module is imported
without a parent URL and resolved with the current process's working
directory.

PR-URL: #48655
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@legendecas legendecas closed this Nov 13, 2023
@legendecas
legendecas deleted the shadow-realm/module branch November 13, 2023 14:11
This was referenced Nov 14, 2023
test-shadow-realm-gc-module can crash on macOS (OOM) #50726
Closed
src: iterate on import attributes array correctly #50703
Merged
@joyeecheung joyeecheung mentioned this pull request Nov 16, 2023
parallel/test-worker-nearheaplimit-deadlock is flaky #50761
Closed
targos pushed a commit that referenced this pull request Nov 23, 2023
@legendecas @targos
src: make process binding data weak
4a1ce45
Avoid the realm being strongly referenced by the process binding data.

PR-URL: #48655
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Nov 23, 2023
@legendecas @targos
src: create worker per isolate properties
5c58341
PR-URL: #48655
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Nov 23, 2023
@legendecas @targos
src: create fs_dir per isolate properties
91aa9dd
PR-URL: #48655
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Nov 23, 2023
@legendecas @targos
src: create per isolate proxy env template
aadff07
PR-URL: #48655
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Nov 23, 2023
@legendecas @targos
module: remove useCustomLoadersIfPresent flag
bc3f7b5
The flag is always true and can be determined by isLoaderWorker solely.

PR-URL: #48655
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request Nov 23, 2023
@legendecas @targos
module: bootstrap module loaders in shadow realm
c7a7493
This bootstraps ESM loaders in the ShadowRealm with
`ShadowRealm.prototype.importValue` as its entry point and enables
loading ESM and CJS modules in the ShadowRealm. The module is imported
without a parent URL and resolved with the current process's working
directory.

PR-URL: #48655
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Nov 28, 2023
v21.3.0 proposal #50954
Merged
RafaelGSS added a commit that referenced this pull request Nov 28, 2023
@RafaelGSS
2023-11-29, Version 21.3.0 (Current)
0049dbe
Notable changes:

crypto:
  * update root certificates to NSS 3.95 (Node.js GitHub Bot) #50805
deps:
  * add simdjson (Yagiz Nizipli) #50322
fs:
  * add c++ fast path for writeFileSync utf8 (CanadaHonk) #49884
module:
  * merge config with `package_json_reader` (Yagiz Nizipli) #50322
  * (SEMVER-MINOR) bootstrap module loaders in shadow realm (Chengzhong Wu) #48655
  * (SEMVER-MINOR) remove useCustomLoadersIfPresent flag (Chengzhong Wu) #48655
src:
  * (SEMVER-MINOR) add `--disable-warning` option (Ethan Arrowood) #50661
  * move package resolver to c++ (Yagiz Nizipli) #50322
  * (SEMVER-MINOR) create per isolate proxy env template (Chengzhong Wu) #48655
  * (SEMVER-MINOR) create fs_dir per isolate properties (Chengzhong Wu) #48655
  * (SEMVER-MINOR) create worker per isolate properties (Chengzhong Wu) #48655
  * (SEMVER-MINOR) make process binding data weak (Chengzhong Wu) #48655

PR-URL: #50954
RafaelGSS added a commit that referenced this pull request Nov 28, 2023
@RafaelGSS
2023-11-29, Version 21.3.0 (Current)
6379d60
Notable changes:

crypto:
  * update root certificates to NSS 3.95 (Node.js GitHub Bot) #50805
deps:
  * add simdjson (Yagiz Nizipli) #50322
fs:
  * add c++ fast path for writeFileSync utf8 (CanadaHonk) #49884
module:
  * merge config with `package_json_reader` (Yagiz Nizipli) #50322
  * (SEMVER-MINOR) bootstrap module loaders in shadow realm (Chengzhong Wu) #48655
  * (SEMVER-MINOR) remove useCustomLoadersIfPresent flag (Chengzhong Wu) #48655
src:
  * (SEMVER-MINOR) add `--disable-warning` option (Ethan Arrowood) #50661
  * move package resolver to c++ (Yagiz Nizipli) #50322
  * (SEMVER-MINOR) create per isolate proxy env template (Chengzhong Wu) #48655
  * (SEMVER-MINOR) create fs_dir per isolate properties (Chengzhong Wu) #48655
  * (SEMVER-MINOR) create worker per isolate properties (Chengzhong Wu) #48655
  * (SEMVER-MINOR) make process binding data weak (Chengzhong Wu) #48655

PR-URL: #50954
RafaelGSS added a commit that referenced this pull request Nov 29, 2023
@RafaelGSS
2023-11-30, Version 21.3.0 (Current)
1f0ba08
Notable changes:

crypto:
  * update root certificates to NSS 3.95 (Node.js GitHub Bot) #50805
fs:
  * add c++ fast path for writeFileSync utf8 (CanadaHonk) #49884
module:
  * (SEMVER-MINOR) bootstrap module loaders in shadow realm (Chengzhong Wu) #48655
  * (SEMVER-MINOR) remove useCustomLoadersIfPresent flag (Chengzhong Wu) #48655
src:
  * (SEMVER-MINOR) add `--disable-warning` option (Ethan Arrowood) #50661
  * (SEMVER-MINOR) create per isolate proxy env template (Chengzhong Wu) #48655
  * (SEMVER-MINOR) create fs_dir per isolate properties (Chengzhong Wu) #48655
  * (SEMVER-MINOR) create worker per isolate properties (Chengzhong Wu) #48655
  * (SEMVER-MINOR) make process binding data weak (Chengzhong Wu) #48655

PR-URL: #50954
RafaelGSS added a commit that referenced this pull request Nov 30, 2023
@RafaelGSS
2023-11-30, Version 21.3.0 (Current)
b7d2827
Notable changes:

crypto:
  * update root certificates to NSS 3.95 (Node.js GitHub Bot) #50805
fs:
  * add c++ fast path for writeFileSync utf8 (CanadaHonk) #49884
module:
  * (SEMVER-MINOR) bootstrap module loaders in shadow realm (Chengzhong Wu) #48655
  * (SEMVER-MINOR) remove useCustomLoadersIfPresent flag (Chengzhong Wu) #48655
src:
  * (SEMVER-MINOR) add `--disable-warning` option (Ethan Arrowood) #50661
  * (SEMVER-MINOR) create per isolate proxy env template (Chengzhong Wu) #48655
  * (SEMVER-MINOR) create fs_dir per isolate properties (Chengzhong Wu) #48655
  * (SEMVER-MINOR) create worker per isolate properties (Chengzhong Wu) #48655
  * (SEMVER-MINOR) make process binding data weak (Chengzhong Wu) #48655

PR-URL: #50954
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
@legendecas @UlisesGascon
src: make process binding data weak
e22a87a
Avoid the realm being strongly referenced by the process binding data.

PR-URL: #48655
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
@legendecas @UlisesGascon
src: create per isolate proxy env template
c78e809
PR-URL: #48655
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Dec 12, 2023
v20.11.0 proposal #51124
Merged
@legendecas legendecas mentioned this pull request Dec 20, 2023
[v20.x] module: bootstrap module loaders in shadow realm #51239
Closed
@joyeecheung joyeecheung mentioned this pull request Jan 12, 2024
lib: remove unnecessary refreshHrtimeBuffer() #51446
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharb ljharb ljharb left review comments
@GeoffreyBooth GeoffreyBooth GeoffreyBooth left review comments
@aduh95 aduh95 aduh95 approved these changes
@mcollina mcollina mcollina approved these changes
@jasnell jasnell jasnell approved these changes
@joyeecheung joyeecheung joyeecheung approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

lib / src Issues and PRs involving general changes in the lib/ or src/ directories. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. realm Issues and PRs related to the ShadowRealm API and node::Realm. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@legendecas @nodejs-github-bot @GeoffreyBooth @joyeecheung @ljharb @mcollina @jasnell @aduh95
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.