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: change idempotency for HostImportModuleDynamically #1645

Merged
merged 1 commit into from
Aug 18, 2019

Conversation

domenic
Copy link
Member

@domenic domenic commented Jul 25, 2019

This better matches HostResolveImportedModule, which only requires idempotency when it completes normally (which is analogous to the success case here).

Closes tc39/proposal-dynamic-import#80, at least on the ECMAScript side.


To be presented at TC39 today; I thought it'd be easier to have a concrete needs-consensus PR rather than trying to page through tc39/proposal-dynamic-import#80.

@ljharb ljharb added needs consensus This needs committee consensus before it can be eligible to be merged. normative change Affects behavior required to correctly evaluate some ECMAScript source text labels Jul 25, 2019
@ljharb ljharb requested review from zenparsing and a team July 25, 2019 18:16
@ljharb ljharb added needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 has consensus This has committee consensus. and removed needs consensus This needs committee consensus before it can be eligible to be merged. labels Jul 25, 2019
@ljharb ljharb requested a review from leobalter July 25, 2019 20:11
@ljharb ljharb self-assigned this Aug 8, 2019
@ljharb
Copy link
Member

ljharb commented Aug 8, 2019

Are there test262 tests for this change? (or, is it hard to test/not currently tested?)

@Snapstromegon
Copy link

@ljharb under language/expressions/dynamic-import are at least the following tests affected by this change:

  • double-error-resolution
  • double-error-resolution-promise

Since this PR removes required idempotency for double error, I think that those test are no longer needed.

Also double-error-resolution_FIXTURE is no longer necessary if the tests above are removed.

@ljharb
Copy link
Member

ljharb commented Aug 13, 2019

@Snapstromegon in that case, a merged test262 PR for those changes would be needed here. Want to file it? :-D

leobalter pushed a commit to tc39/test262 that referenced this pull request Aug 13, 2019
Due to changing idempotenxy for dynamic import on failures these tests are no longer needed.
After tc39/ecma262#1645 idempotency is only required after completing normally.
@ljharb ljharb added has test262 tests and removed needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Aug 13, 2019
@ljharb ljharb force-pushed the module-idempotency-alignment branch from 82854a7 to d417f5d Compare August 18, 2019 07:03
)

This allows failures to become successes on later calls. This better matches HostResolveImportedModule, which only requires idempotency when it completes normally (which is analogous to the success case here).

Closes tc39/proposal-dynamic-import#80, at least on the ECMAScript side.
@fatso83
Copy link

fatso83 commented May 8, 2023

Where did this change actually end up? I had a look at the rendered output of https://tc39.es/proposal-dynamic-import/ and it only contained the old text, before this change. Then I tried searching the actual speco of ecma-262, but found hits on either pre or post text.

The most relevant sections seems to be 13.3.10 Import Calls and HostLoadImportedModule

@nicolo-ribaudo
Copy link
Member

We unified the HostResolveImportedModule and HostImportModuleDynamically hooks, the requirement is now the second bullet point of https://tc39.es/ecma262/#sec-HostLoadImportedModule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has consensus This has committee consensus. has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Retry failed dynamic import
7 participants