Lift Loader.parse into System.parse.

[johnjbarton]

This allows Loader to be independent of compiler/transcoder and
it allows the System to control parse options w/o consulting Loader.

[guybedford]

I'm not sure I like the idea of coupling parse to System over Loader. It also seems to mix up the private and spec API's in a way that might lead to it being perceived as a spec hook. Could we not set it as Loader.parse or LoaderPolyfill.parse instead?

[johnjbarton]

Regarding the last comment here, I can't tell if you mean "I don't like System to own parse" or "I don't like setting parse via Loader hooks argument", so I will try to address both cases.

Absent the spec, would you consider this new split between System and Loader a better design? I would, because the concerns of the Loader are now entirely focused on linking and no other issue. For module linking, parse is a necessary operation like fetch, with a complex implementation but low coupling to linking (the API for parse is narrow and can be described easily). Pulling this function out of Loader makes the overall design better. I'll make this case to es-discuss.

As to your concern that this function may perceived as part of the spec, as far as I can tell the spec does not prohibit this kind of extension. Let's consider the two possibilities: 1) one or more downstream consumers of this Loader uses the parse() hook or 2) no downstream consumer uses it. The first case is really just more evidence that parse belongs in the spec API. In the second case no harm is done by adding it here. I don't think anyone will accidentally implement a ES6 parser because they saw this extended API ;-).

I suppose we could make the API "you must monkey patch Load.parse" but that seems like it does not help any one using the code.

Does this convince you?

[guybedford]

Thanks this looks great.

The issue with making parse a spec function is it means opening up the parse tree to be something that is specified. We get around returning the syntax tree directly here through the System.register implementation which simply sets a declare function, providing our alternative handling of circular references. But in the spec, the syntax tree is parsed and stored in the specific form for the engine. As a result, I don't think this is something that can be standardised as easily.

When considered from the spec perspective, the parse tree is actually stored under load.body, so that the Loader implementation is heavily coupled to the parse tree itself.

That's why I was suggesting something like:

Loader.parse = ...

So that it can both be a private API, and coupled to the loader.

[guybedford]

With this PR we should also put some thought to #171. These global overwrites need to be given a little careful attention.

[johnjbarton]

As far as I can tell from reading the spec, [[Body]] is an opaque typed property, created by parsing and consumed by evaluate. In 15.2.6.2 EnsureEvaluated(mod, seen, loader) Abstract Operation:

Let r be the result of evaluating mod.[[Body]].

So the spec need make no rules about the content of [[Body]], only that it is set on to load.body by parse. (I think parse needs a companion function to evaluate the load.body in a realm).

Currently we are parsing and compiling to a function object passed on load.declare, rather than parsing to a [[Body]] and passing it on load.body. Since there is nothing in the spec about [[Body]] being a tree I think we could set load.body rather than load.declare.

[johnjbarton]

PTAL

[guybedford]

Alright, this seems great. It would be nice to have some docs on parse and register but I'll be sure this is updated in due course as well.

Can we rebase?

[johnjbarton]

I guess you meant "Can you squash the commits"? Anyway that is what I did.