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

Normative: Define module root level context #98

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Jul 15, 2024

This defines the root level context for a SourceTextModule. This makes the surrounding context consistent and predictable when module evaluation is either deferred, or dynamic. When a module evaluation is deferred, the module's body only gets evaluated when its export entries get accessed. However, this depends on if the module contains TLA. Hosts, that want to expose the originating context of a module, can expose the context via HostGetImportMetaProperties, and HostFinalizeImportMeta.

Notably, this doesn't change the context of ScriptEvaluation, as it is used in hosts like https://html.spec.whatwg.org/#timers:run-a-classic-script.

Fixes #93

spec.html Outdated
</dl>
<p>An implementation of GetHostDefinedAsyncContextMapping must conform to the following requirements:</p>
<ul>
<li>It must perform and return a List of Async Context Mapping Records.</li>
Copy link
Member

Choose a reason for hiding this comment

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

Another requirement would be to always return the same mapping.

Copy link
Member

Choose a reason for hiding this comment

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

or in the opposite direction, pass the module record to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated with a parameter module records and the result list must contain the same mapping for the given module record.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand this change. I thought the goal of this PR was to make is so that all modules have the same top-level mapping, while this PR is now defining the top-level mapping as potentially different for each module.

Copy link
Member Author

@legendecas legendecas Jul 16, 2024

Choose a reason for hiding this comment

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

This allows hosts to add mapping entries based on the origin of the module records. The goal of the PR is allowing hosts initialize their entries, instead of deriving the mappings based on the originator while an imported module is only evaluated once.

Copy link
Member

Choose a reason for hiding this comment

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

So this allows hosts to implement both fresh-context-per-module and module-based-on-importer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what you mean by module-based-on-importer, but my understanding is that if two different importers import the same module, you'd still only evaluate once, and any importer dependency would resolve based on one or the other (presumably the earlier one).

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Jul 16, 2024

Choose a reason for hiding this comment

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

Right, the original motivation of this PR was to give an answer to this question: what is the context of the module in this case?

variable.get(); // undefined

variable.run(1, () => import("x"));

At the top-level of x, does variable.get() return undefined or 1? This PR as-is does not give an answer, and instead laves it host-defined.

To give an answer, this PR could either:

  • not do anything, and import("x") would see 1
  • have a non-per-module top-level snapshot (stored in the realm record?), and import("x") would see undefined

With this PR both behaviors are possible:

  • if the host implements HostGetTopLevelAsyncContextMapping(module) as "Return AsyncContextSnapshot()", then it's 1
  • if the host implements HostGetTopLevelAsyncContextMapping as "Return the original top-level snapshot", then it's undefined

spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
Copy link
Contributor

@shicks shicks left a comment

Choose a reason for hiding this comment

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

A couple nits, but overall it looks good.

<p>An implementation of HostGetTopLevelAsyncContextMapping must conform to the following requirements:</p>
<ul>
<li>It must return a List of Async Context Mapping Records.</li>
<li>The result List must contain the same Async Context Mapping Records for a given module record.</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, HostHasSourceTextAvailable has a similar requirement worded as "It must be deterministic with respect to its parameters. Each time it is called with a specific func as its argument, it must return the same result". It might be worth rephrasing to be consistent with that language.

</dl>
<p>An implementation of HostGetTopLevelAsyncContextMapping must conform to the following requirements:</p>
<ul>
<li>It must return a List of Async Context Mapping Records.</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

I still think this requirement should be deleted, since it's completely redundant with the signature. Looking through the other host-defined ops, I see a few with non-unused returns (e.g. HostHasSourceTextAvailable and HostResizeArrayBuffer) and none of them have this sort of restatement of the signature as an enumerated requirement.

<p>An implementation of HostGetTopLevelAsyncContextMapping must conform to the following requirements:</p>
<ul>
<li>It must return a List of Async Context Mapping Records.</li>
<li>The result List must contain the same Async Context Mapping Records for a given module record.</li>
Copy link
Member

Choose a reason for hiding this comment

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

This host hook is only called once per module record, so this requirement is not necessary.

@legendecas legendecas changed the title Define module root level context Normative: Define module root level context Jul 29, 2024
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.

Initial AsyncContext.Snapshot
5 participants