Skip to content
This repository was archived by the owner on Apr 2, 2026. It is now read-only.

refactor: Move ModuleMap to JsRealm#45

Merged
bartlomieju merged 4 commits into
denoland:mainfrom
bartlomieju:move_module_map_to_jsrealm
Jul 19, 2023
Merged

refactor: Move ModuleMap to JsRealm#45
bartlomieju merged 4 commits into
denoland:mainfrom
bartlomieju:move_module_map_to_jsrealm

Conversation

@bartlomieju

Copy link
Copy Markdown
Member

Extracted from #41.

Now all JsRealm instances have a ModuleMap, but only the "main realm"
ModuleMap is being used.

@bartlomieju bartlomieju requested a review from mmastrac July 18, 2023 22:58
@bartlomieju

Copy link
Copy Markdown
Member Author

CC @andreubotella

Comment thread core/runtime/jsruntime.rs Outdated
Comment on lines 1621 to 1623
// `self.module_map` points directly to the v8 isolate data slot, which
// we must explicitly drop before destroying the isolate. We have to
// take and drop this `Rc` before that.

@andreubotella andreubotella Jul 18, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This comment should probably be removed now.

@andreubotella andreubotella left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@bartlomieju bartlomieju force-pushed the move_module_map_to_jsrealm branch from 71b7c71 to 3b1e526 Compare July 18, 2023 23:24
@bartlomieju bartlomieju enabled auto-merge (squash) July 18, 2023 23:25

@mmastrac mmastrac left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@bartlomieju bartlomieju merged commit fe93b1d into denoland:main Jul 19, 2023
@bartlomieju bartlomieju deleted the move_module_map_to_jsrealm branch July 19, 2023 00:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants