Skip to content

Conversation

@andreubotella
Copy link
Contributor

@andreubotella andreubotella commented Jul 17, 2023

TODO: Detailed description.

This PR is a port of denoland/deno#15760.

Towards #911.

@bartlomieju
Copy link
Member

Alright, I think we can land this PR now :) awesome work @andreubotella.

@nayeemrmn please take a look as well

@bartlomieju bartlomieju self-requested a review July 20, 2023 21:10
Copy link
Collaborator

@nayeemrmn nayeemrmn left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 866 to 867
// TODO(andreubotella): Should preserve_snapshotted_modules be in CreateRealmOptions?
self.init_extension_js(&realm, None, None)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a problem with using preserve_snapshotted_modules from the runtime options on each realm? Since realms apply the snapshot implicitly and this is just another snapshot option.

Copy link
Contributor Author

@andreubotella andreubotella Jul 21, 2023

Choose a reason for hiding this comment

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

I don't really have much of a clear idea at this point of what preserve_snapshotted_modules does in detail or how it's supposed to be used, other than that deno_runtime uses it for node: modules. I don't know if it makes sense to keep the option from runtime initialization.

Edit: The realm creation options let you pass a module loader, which will often be the same as the isolate's (that will be the case for ShadowRealm, for example), but for embedders it need not be (as <iframes> have different module maps in the web). I'm not too sure how that would interact with preserved_snapshotted_modules.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It makes all of the deno_<ext>: modules non-importable for users by removing them from the module map, but accepts a list of modules to exempt from that i.e. node: modules in our case. It's a step which is just as applicable to every realm.

Copy link
Contributor Author

@andreubotella andreubotella Jul 21, 2023

Choose a reason for hiding this comment

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

While trying to understand how the snapshotted modules interact with the embedder-provided module loader, I noticed that currently they are not included at all in the realm's module map:

// TODO(andreubotella): Should the module map be initialized with snapshotted data?

(that was added in PR #45, which was split off this one)

I will update this PR to include those snapshotted modules in the module map, and to support preserve_snapshotted_modules.

Copy link
Contributor Author

@andreubotella andreubotella Jul 22, 2023

Choose a reason for hiding this comment

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

I think that should probably be a follow-up PR though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will revert the change that moves the code that handles preserved_snapshotted_modules inside init_extension_js, since it is now a lot clearer to me that it doesn't belong there. I will handle that in new_inner and create_realm in the follow-up PR.

Copy link
Member

@bartlomieju bartlomieju left a comment

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 b9cf47d into denoland:main Jul 22, 2023
@andreubotella andreubotella deleted the modules-in-realms branch August 4, 2023 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants