Implementation feedback

[guybedford]

I've got a working implementation at https://github.com/ModuleLoader/es6-module-loader/blob/1.0/src/loader.js (PR ModuleLoader/es-module-loader#317).

Here are some notes of the minor corrections from the process:

• 4.1.5 1 Instantiation must be called with a loader argument.
• 4.1.5 1 instance is both an argument and a local variable.
• 4.1.5 4 Resolve entry.[[Instantiate]] with instance. We're resolving the promise from outside of the scope of the promise which practically, means storing the instantiateResolve function on the entry. It can be easier to move this out to a return entry.module in 4.2.3 at the instantiate promise return and a Promise.resolve in the last line of 4.1.4.
• 4.1.5 6 b For each dep in instance.[[ImportedModules]] is this a ReflectiveModuleRecord property? I couldn't see it defined anywhere.
• 4.2.1 6 use payload instead of v for consistency?
• 4.2.3 5 Should RequestTranslate directly, don't need RequestFetch as it is implicit in RequestTranslate
• 4.2.3 Let p1 be the result of promise-calling hook(key, payload, source)
  Surely we just need source and not payload?
• 4.2.4 entry intead of entry0
• 4.2.4 Resolve is not defined anywhere? loader.[[resolve]]?
• 4.2.4 Before running RequestInstantiate for dependencies, we need to do the check:
  _depEntry_ = ensureRegistered(loader, depKey, metadata); if _depEntry_.[[state]] is "ready" then use _depEntry_ for _dep_
  Otherwise requestInstantiate throws if the module is already defined
• 4.2.5 module.Evaluate is not defined?
  Resolve is not defined?
• 5.2.1 Let status be module.Instantiate(). I'm wondering about using the module object as both the execute function and the module instance?
• 6.3.1 To confirm - if stage is fetch or translate, it never proceeds to next stage?
• 6.4.2 Not sure how loader.prototype.error is to be implemented.
• 6.5.2 Should probably set error property to null for installed entry.
• 6.5.3 Reflect.loader.uninstall(key) not Reflect.loader.uninstall(module)

I also implemented the metadata piping. Having metadata available as soon as the resolve hook runs would really be the ideal here, but does mean that the first resolution takes precedence, which I know may not be ideal for the spec process.

To describe the implementation (for what its worth):

• We add a third metadata argument to resolve, and pass it through to the request* calls
• 4.1.1 Added metadata last argument to ensureRegistered. If already existing, it ignores, otherwise it sets on the entry if given, or an empty object
• 4.2.1 - 4.2.5 all take metadata last argument and pass to ensureRegistered
• 6.3.1 May also have to consider metadata option for Loader.load
• 6.5.1 Lookup to return metadata as well.

If it would help to provide any of the above as a PR I'd be more than happy to assist, but would rather have the suggestions reviewed at a higher level first.

[guybedford]

Note #25 is the only critical bug from the process.

[caridy]

@guybedford this a very valuable feedback. I did a first pass, and fixed some of the cosmetics in PR #26, waiting for @dherman for the heavy stuff. Here are the details:

4.1.5 1 Instantiation must be called with a loader argument.

PR. CommitInstantiated is called from RequestInstantiate and ResolveInstantiate, which they both have access to loader. It needs loader to call Instantiation().

4.1.5 1 instance is both an argument and a local variable.

PR, renamed to _module_

4.1.5 4 Resolve entry.[[Instantiate]] with instance. We're resolving the promise from outside of the scope of the promise which practically, means storing the instantiateResolve function on the entry. It can be easier to move this out to a return entry.module in 4.2.3 at the instantiate promise return and a Promise.resolve in the last line of 4.1.4.

this one is for @dherman.

4.1.5 6 b For each dep in instance.[[ImportedModules]] is this a ReflectiveModuleRecord property? I couldn't see it defined anywhere.

I haven't see ImportedModules anywhere, this is for @dherman

4.2.1 6 use payload instead of v for consistency?

PR

4.2.3 5 Should RequestTranslate directly, don't need RequestFetch as it is implicit in RequestTranslate

the call to RequestFetch cannot be skipped because we need _payload_ when calling _loader_.[[Instantiate]]

4.2.3 Let p1 be the result of promise-calling hook(key, payload, source) Surely we just need source and not payload?

Not sure about this one. @dherman

4.2.4 entry intead of entry0

PR

4.2.4 Resolve is not defined anywhere? loader.[[resolve]]?

maybe Let _hook_ be _loader_.[[Resolve]].. @dherman

4.2.4 Before running RequestInstantiate for dependencies, we need to do the check: depEntry = ensureRegistered(loader, depKey, metadata); if depEntry.[[state]] is "ready" then use depEntry for dep Otherwise requestInstantiate throws if the module is already defined

PR

4.2.5 module.Evaluate is not defined? Resolve is not defined?
5.2.1 Let status be module.Instantiate(). I'm wondering about using the module object as both the execute function and the module instance?
6.3.1 To confirm - if stage is fetch or translate, it never proceeds to next stage?
6.4.2 Not sure how loader.prototype.error is to be implemented.
6.5.2 Should probably set error property to null for installed entry.

all the above is for @dherman

6.5.3 Reflect.loader.uninstall(key) not Reflect.loader.uninstall(module)

PR

[dherman]

OK, I started by merging Caridy's PR (thanks Caridy!). Now attempting to make this a task list so I can track my progress:

• 4.1.5 1 Instantiation must be called with a loader argument.

Fixed by Caridy.

• 4.1.5 1 instance is both an argument and a local variable.

Fixed by Caridy and renamed again in 0916391.

• 4.1.5 4 Resolve entry.[[Instantiate]] with instance. We're resolving the promise from outside of the scope of the promise which practically, means storing the instantiateResolve function on the entry. It can be easier to move this out to a return entry.module in 4.2.3 at the instantiate promise return and a Promise.resolve in the last line of 4.1.4.

Discussed and resolved below.

• 4.1.5 6 b For each dep in instance.[[ImportedModules]] is this a ReflectiveModuleRecord property? I couldn't see it defined anywhere.

No, it's a Source Text Module Record property but I got the name wrong. Fixed in 8758a8a.

• 4.2.1 6 use payload instead of v for consistency?

Fixed by Caridy.

• 4.2.3 5 Should RequestTranslate directly, don't need RequestFetch as it is implicit in RequestTranslate

No, as Caridy explained.

• 4.2.3 Let p1 be the result of promise-calling hook(key, payload, source) Surely we just need source and not payload?

Yeah, seems fine. You can always propagate earlier stage data via metadata. Fixed in fe60901.

• 4.2.4 entry intend of entry0

Fixed by Caridy.

• 4.2.4 Resolve is not defined anywhere? loader.[[resolve]]?

Defined in de47c19.

• 4.2.4 Before running RequestInstantiate for dependencies, we need to do the check: depEntry = ensureRegistered(loader, depKey, metadata); if depEntry.[[state]] is "ready" then use depEntry for dep Otherwise requestInstantiate throws if the module is already defined

Partially fixed by Caridy; needs fix to issue #25.

• 4.2.5 module.Evaluate is not defined? Resolve is not defined?

It's ModuleEvaluation. Fixed in 0a90264.

• 5.2.1 Let status be module.Instantiate(). I'm wondering about using the module object as both the execute function and the module instance?

An instance is either a factory function or a module instance object. The first half of Link() executes all the factory functions, so all we have left are module instance objects.

• 6.3.1 To confirm - if stage is fetch or translate, it never proceeds to next stage?

Confirm. The point of the stage argument is to say you are only requesting it run to a certain point. This gives programmers control over both side effects and performance (e.g. for pre-fetching).

• 6.4.2 Not sure how loader.prototype.error is to be implemented.
• 6.5.2 Should probably set error property to null for installed entry.

Filed these two as a separate issue #31.

• 6.5.3 Reflect.loader.uninstall(key) not Reflect.loader.uninstall(module)

Fixed by Caridy.

[dherman]

@guybedford See the question ^^

[dherman]

PS thank you for this detailed review!

[guybedford]

@dherman sure, glad to be able to help! The issue I was referring to in 4.1.5 4 was that entry.instantiate is a promise that was created in another scope that we are resolving in this scope.

// in one scope
entry.instantiate = new Promise(...);

// in another scope
entry.instantiateResolve(value);

The only way to resolve a promise from outside its scope of creation is to store the resolve function itself on the entry as above.

The suggestion was a code reworking to avoid the need to do this, but it's a code detail not a spec detail so may be irrelevant to the spec.

[dherman]

@guybedford The reason that has to be exposed is for L.p.provide. Essentially we want to allow frameworks that configure the loader to race it. For example if you have a package already loaded that may provide the source, you can provide it early and race the pre-existing fetch promise.

[guybedford]

@dherman right thanks ok, so this affects all fulfill handlers equally not just that one, and is a key part of the mechanism. I'll fix the implementation this side to handle store all the resolve methods for external access then - I think we can consider this resolved then!