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

Editorial: Provide context for HostImportModuleDynamically requirement #1598

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

Conversation

mikesamuel
Copy link
Member

I had a hard time figuring out the stability requirement:

Every call to HostImportModuleDynamically with the same
referencingScriptOrModule and specifier arguments must conform
to the same set of requirements above as previous calls
do. That is, if the host environment takes the success path once for
a given referencingScriptOrModule, specifier pair, it must
always do so, and the same for the failure path.

This adds an editorial note gleaned from discussions on #tc39.

If accurate, it may help others.

@devsnek who explained the context.
@littledan @domenic who probably drafted/championed the requirement.

I had a hard time figuring out the stability requirement:

> Every call to HostImportModuleDynamically with the same
> *referencingScriptOrModule* and *specifier* arguments must conform
> to the <em>same</em> set of requirements above as previous calls
> do. That is, if the host environment takes the success path once for
> a given *referencingScriptOrModule*, *specifier* pair, it must
> always do so, and the same for the failure path.

This adds an editorial note gleaned from discussions on #tc39.

If accurate, it may help others.
@@ -22658,6 +22658,9 @@ <h1>Runtime Semantics: HostImportModuleDynamically ( _referencingScriptOrModule_
</li>
<li>
Every call to HostImportModuleDynamically with the same _referencingScriptOrModule_ and _specifier_ arguments must conform to the <em>same</em> set of requirements above as previous calls do. That is, if the host environment takes the success path once for a given _referencingScriptOrModule_, _specifier_ pair, it must always do so, and the same for the failure path.
<emu-note>
<p>Changes to module caches that are exposed to user code (e.g. `delete require.cache[k]`) must not affect dynamic imports.</p>

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 follow what this is trying to capture? require.cache[k] implies a k being a unique key in a module registry, which is not a concept ECMA-262 deals with at all. Whether a separate registry key change results in changes to dynamic import is not an ECMA-262 concern, rather the ECMA-262 concern is surely already captured that for a given specifier and referencing module, the same resolved module should always be provided. How is this unclear already?

Copy link
Member

Choose a reason for hiding this comment

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

I agree that we should probably leave require out of it. Perhaps that particular requirement can be stated in a more approachable way (or be merged in with with the previous requirement to which it references).

Copy link
Member Author

Choose a reason for hiding this comment

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

This is meant to provide context by referencing widely understood external systems.

I could try to rephrase to refer to something like https://nodejs.org/api/modules.html#modules_require_cache

By deleting a key value from this object, the next require will reload the module.

Adding or replacing entries is also possible.

But that still references something external. Providing context relating to quirks of parallel mechanisms seems hard without external reference.

@ljharb ljharb force-pushed the master branch 3 times, most recently from 3d0c24c to 7a79833 Compare June 29, 2021 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants