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

Layering: Add GetRequestedModules() to Module Record #1501

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@dandclark
Copy link

dandclark commented Apr 4, 2019

Add the GetRequestedModules() abstract method to Abstract Module Record, with a corresponding implementation for Source Text Module Record. Move Cyclic Module Record's [[RequestedModules]] down to Source Text Module Record, as it is now used directly only by STMR, and its presence on Cyclic MR causes problems for derived module types like HTML Module Record that don't necessarily have strings associated with all their child modules.

The purpose of GetRequestedModules() is to abstract out from InnerModuleInstantiation/InnerModuleEvaluation the details of how a Module Record type stores its requested modules, since these are different for HTML Module Records as implemented here.

It is not time yet to merge this in given that there are still open questions about the HTML Modules design (mostly tracked over in the w3c/webcomponents repo). The intent here is to facilitate discussion and tease out issues that can be better found when making changes against the actual spec, with the intention of eventually merging in the updated PR once consensus has been reached on all open issues.

I've placed the built result of this change here: https://dandclark.github.io/built-specs/ecma262/get-requested-modules.html

Add GetRequestedModules() abstract method to Cyclic Module Record, wi…
…th implementation for Source Text Module Record. Refactor InnerModuleInstantiation/InnerModuleEvaluation to use this instead of [[RequestedModules]] directly.
Show resolved Hide resolved spec.html Outdated
Show resolved Hide resolved spec.html Outdated
@littledan
Copy link
Member

littledan left a comment

This change seems good to me. It'll cause a tiny bit of duplication for WebAssembly modules, but that's fine.

A nit: Maybe [[RequiredModules]] should move down to SourceTextModuleRecord if its usages are exclusively there.

Thanks for following up with these appropriate layering changes; I am excited to see HTML modules moving forward.

1. Let _module_ be this Source Text Module Record.
1. Let _requestedModules_ be an empty List of Abstract Module Records.
1. For each String _requested_ that is an element of _module_.[[RequestedModules]], do
1. Let _requestedModule_ be ! HostResolveImportedModule(_module_, _requested_).

This comment has been minimized.

Copy link
@littledan

littledan Apr 5, 2019

Member

The specification previously had ? rather than ! from some paths, so I think it would be best to leave it that way here. Then, the two callsites can invoke it with either of those, based on whether they anticipate errors.

This comment has been minimized.

Copy link
@dandclark

dandclark Apr 5, 2019

Author

Thanks! I like the idea of moving [[RequiredModules]] down to STMR...I'll look into this.

This comment has been minimized.

Copy link
@dandclark

dandclark Apr 15, 2019

Author

I pushed an update that:

  • Switched '?' to '!' in GetRequestedModules and removed the note stating that the HostResolveImportedModules call must have succeeded.
  • Pushed [[RequestedModules]] down to Source Text Module Record.

Thanks for the comments!

@littledan

This comment has been minimized.

Copy link
Member

littledan commented Apr 8, 2019

Nit: I see this marked as "Normative" in the PR title. However, I don't know what normative changes are in this PR. To me, it looks more like a "Layering" (or "Editorial") PR, which should be possible to land without tests or prior review in plenary (unless it seems controversial or worth noting in committee).

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Apr 8, 2019

Layering PRs are often normative; and we don’t have a different commit category for them. In particular, almost any change to Modules is effectively normative due to the partial nature in which 262 specifies them.

@littledan

This comment has been minimized.

Copy link
Member

littledan commented Apr 8, 2019

Just for context, we have used the "Layering:" prefix on commits before, e.g., #242 , even if I failed to list this when writing our current CONTRIBUTING.md. I still don't understand how this patch is normative, in a way that would make other layering patches not normative (since embedders can hook into any of the layering changes). I don't expect any JavaScript programs to change their behavior due to this change.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Apr 8, 2019

That's a fair point, and maybe that makes this one editorial; either way, I'd expect anything that impacts Modules, especially CMRs and STMRs, to require committee consensus (otherwise the "dynamic modules" agenda item wouldn't have required it either).

dandclark added some commits Apr 15, 2019

Move Cyclic Module Record's [[RequestedModules]] list down to Source …
…Text Module Record. The base type will now always use GetRequestedModules() to obtain this list.

@dandclark dandclark changed the title Normative: Add GetRequestedModules() to Module Record Layering: Add GetRequestedModules() to Module Record Apr 15, 2019

@dandclark

This comment has been minimized.

Copy link
Author

dandclark commented Apr 15, 2019

Yeah, when creating the PR I thought it didn't quite fit into either 'Normative' or 'Editorial' but didn't know there was precedent for using 'Layering' as a category. :) I've updated the title to 'Layering'.

In any case I agree that a module chage like this likely requires committee consensus.

@littledan
Copy link
Member

littledan left a comment

I don't see any issues with this PR. Good work! However, maybe we should wait to land it until there's a bit more clarity on what the semantics of HTML modules will be.

@zenparsing

This comment has been minimized.

Copy link
Member

zenparsing commented Apr 17, 2019

I'm curious if the desired indirection cannot instead be applied in HostResolveImportedModule.

The [[RequestedModules]] list can be any arbitrary strings: their semantics depends upon the module record that contains them. A cyclic module record m could have auto-generated keys in [[RequestedModules]], say #1 and #2. HostResolveImportedModule(m, '#1') would then resolve to the desired dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.