tc39 / proposal-built-in-modules

BSD 2-Clause "Simplified" License
892 stars 25 forks source link

Add specification for Synthetic Module Records #44

Closed domenic closed 1 year ago

domenic commented 5 years ago

Hey folks!

I thought I'd get things started with a spec for module records that seem like they'd be useful for built-in modules, JSON modules, CSS modules, etc. As we work through these ideas on the web platform side, we've found some confusions that would be prevented with a more concrete spec text, so I wanted to write something up we could point to.

I've deployed a preview of the built product at https://proposal-javascript-standard-library-domenic.now.sh/. Thoughts welcome!

littledan commented 5 years ago

This looks great to me. I don't see any particular technical issues, just some one editorial comment (which would not be any sort of normative change):

All use cases I can think of for synthetic modules would have fixed exports once the module is "parsed". What if CreateSyntheticModule instead took two arguments, the realm and a mapping from strings to JavaScript values (or, since we don't have infra, a List of Records { [[Name]]: String, [[Value]]: JS value }), and set these up as immutable bindings during the Initialize() phase? We can consider generalizing the mechanism if there's a particular use case for it.

cc @ms2ger

domenic commented 5 years ago

Yeah, that's a possible approach. However, my thinking went toward the current approach like so:

  1. We need custom evaluation steps anyway for side effects. (E.g., KV storage throws in non-secure contexts, or built-in module might define a custom element, or apply a stylesheet to the page, or something.) So that hook needs to stay anyway.

  2. It's weird to create objects before the module is evaluated. For example, KV storage isn't supposed to be usable in non-secure contexts. We could create all its exports ahead of time, export them as immutable bindings, but then have the module just throw when it's evaluated, despite exporting things. But that seems strange; I'd prefer to emulate how JS code works, in that it establishes the bindings' values at evaluation time, after any checks it might want to perform. I think they are observably equivalent though, i.e. I think you can't observe a throwing-when-evaluated module's bindings.

    (Note: there are other ways to achieve the secure context restriction, e.g.: not putting the module in the module map at all, or exporting things that throw when you try to use them, or exporting undefined for all exports, or exporting zero bindings in that case. I don't claim the current design is the best one for KV storage to choose, and welcome discussion over on that issue tracker. But, I think taking it as a given for this discussion is a helpful way of illustrating the things a built-in module might want to do, and that any generic system like this PR should probably accomodate.)

  3. It seems at least possible that you might want to export, either mutable bindings, or bindings whose values can't be determined until evaluation time. (For example, given a JSON module, is it OK to execute the JSON parsing algorithm at instantiation time? It seems like it runs JS script to me.) Thus something like SetSyntheticModuleExport() seems useful to include. But if we're going to have that anyway, then why create a dual system where you also have an [[ExportValues]] List, or a { [[Name]], [[Value]] } List, for the initial values, and use SetSyntheticModuleExport() for the runtime ones?

I'm open to changing this, but I think the current approach is more flexible, so I'm inclined toward that one.

domenic commented 5 years ago

To be more concrete, with the JSON module example, with the OP design I think it would have straightforward evaluation steps:

  1. Let result be ? Call(%JSONParse%, undefined, « jsonText »).
  2. Perform SetSyntheticModuleExport(module, "default", result).

But with a static-exports approach, it would instead need to be something like:

littledan commented 5 years ago

Thanks for walking through these error cases; that makes the motivation for this design a lot clearer. The strategy you describe for JSON parse errors is sort of what I expected (that's what we do for JS early errors phase-wise, right?), but I can see how it's a lot more complicated to write down.

Actually, now, we came to an observable difference, didn't we? The find the first parse error algorithm would stop any module from executing if it hit a JSON parse error if these are treated as parse errors. But, if they are treated as errors during the execution phase, it might throw the exception after executing some other earlier sibling modules. (Maybe we should move this discussion back to https://github.com/whatwg/html/issues/4315#issuecomment-456548059 .)

All I could find on import-maps for why it throws an exception on evaluation rather than being absent from the import-map (which is what I would've expected, but don't feel strongly about) was https://github.com/WICG/kv-storage/issues/16 . Was there more context somewhere, or should I file a different issue?

littledan commented 5 years ago

Thinking about this more: Even if I am not sure about those semantic details for the use cases, I think it's good to give module evaluation steps, to do the equivalent of executing class declarations, etc. This would make it very practical to have a real implementation which executes synthetic modules based on JavaScript modules. So, +1 to all of this PR from me.

From that frame, I'm wondering: Should we leave the step of initializing bindings to the module, rather than initializing them to undefined?

Ms2ger commented 5 years ago

Hey folks, I'm planning to look into adding this text to WebIDL for the time being, so we can move forward with https://github.com/whatwg/html/pull/4407. I'd be happy to help with moving this into Ecma262 or any other place in the future, if that's what we decide on. I'd certainly want to make sure it remains useful for the work you're doing here.

Ms2ger commented 5 years ago

Please see https://github.com/heycam/webidl/pull/722