Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ModuleRecord to ImportMeta hook to reflect HostGetImportMetaProperties #22

Closed
wants to merge 1 commit into from

Conversation

Jack-Works
Copy link
Member

@Jack-Works Jack-Works commented Oct 22, 2020

Currently, the importMetaHook cannot access the module record.
Add it to the hook can match the HostGetImportMetaProperties and provide more abilities when manipulating import.meta

close #13

@kriskowal
Copy link
Member

This looks like a good change to me. I imagine it’s possible to make this work without passing back the module record, if the load hook tracks the specifier to module record map in a scratch, but this seems like a harmless way to make that more convenient.

When we get into the detailed explanation of how this works, I believe the object passed here should be identical to the object that was returned by the loader hook (currently called importHook).

Whether this is actually useful hinges on other factors. The module record might not carry useful information. The static module record could have a location property, but as we discussed in SES Strategy this morning, that might not be the right source of truth for the import.meta.url. Also, we might not want to carry location on the static module record because it might not be the same between compartments.

Creating a compartment might be less prone to hazard if the importMetaHook is obliged to compute meta from just the specifier and information about the compartment layout.

@Jack-Works
Copy link
Member Author

In my suggestion in #13, I would image the execution order of hooks like this:

HostGetImportMetaProperties(host) => HostFinalizeImportMeta(host) => importMetaHook(compartment)

If so, when the importMetaHook is called, import.meta.url is already set by the host (if they have import.meta.url). If there is no special need, the developer doesn't need to calculate this property again.

@kriskowal
Copy link
Member

We’ve changed a lot since we discussed this PR. Do you still support adding the static module record as an argument to importMetaHook, with the current design?

@Jack-Works
Copy link
Member Author

I cannot recall why I proposed this change, and it looks like it requires to reify of the module record which is may not be wanted. I'll close this for now until I find the use case for this.

@Jack-Works Jack-Works closed this Jun 9, 2022
@Jack-Works Jack-Works deleted the import-meta-hook branch June 9, 2022 03:57
@Jack-Works
Copy link
Member Author

Oh I see my motivation in #13

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.

Receive the module instance in importMetaHook?
2 participants