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

Remove namespace box in promise returned by compartment.import #29

Closed
kriskowal opened this issue Oct 22, 2020 · 12 comments
Closed

Remove namespace box in promise returned by compartment.import #29

kriskowal opened this issue Oct 22, 2020 · 12 comments

Comments

@kriskowal
Copy link
Member

Some time ago, I wrote up the evident design rationale for having import return a promise containing a namespace box.

https://github.com/tc39/proposal-compartments#boxed-module-namespace-returned-by-compartment-import

However, this disagrees with the design principle already established by dynamic import in the scope of a module. I propose that we should make dynamic import consistent with compartment import by returning a promise for the exotic module exports namespace object, without a { namespace } box around it. This does mean that “thenable modules” will be coerced in the resolution of the promise returned by compartment.import and dynamic import, but it would at least be consistent.

@bmeck
Copy link
Member

bmeck commented Oct 22, 2020

I'm not entirely convinced that 1-1 with import expressions is a goal. various other values might be relevant like the completion value of a module, or other data that a host-like is more interested in than a raw JS user. Is there a problem with the inconsistency or is it about convenience?

@kriskowal
Copy link
Member Author

The only motivation here is consistency for the sake of the principle of least astonishment (or, less-WAT in the JS idiom). It is also a bit cumbersome to destructure in practice. For example, from the shim tests:

  const { namespace } = await compartment.import('./main.js');
  t.equal(namespace.default.a, 10);

Or alternately:

  const { namespace: { default: main } } = await compartment.import('./main.js');

Whereas, this would be considerably less cumbersome and also less astonishing:

  const { default: main } = await compartment.import('./main.js');

And of course, without defaults exports, the parallel of static import is more clear:

  import { main } from "./main.js";
  const { main } = await import("./main.js");

But, you point out a motivation I had not inferred and did not catalog. Can you conceive of an example of other properties beyond namespace that would be interesting for import? It strikes me as likely that other host-relevant metadata would be better carried elsewhere. I see import as an end-user facing API and the hooks as where host-relevant metadata is more likely to be exposed.

@bmeck
Copy link
Member

bmeck commented Oct 22, 2020

I know we used to use the completion value of Module evaluation in Node's ESM loader to generate facades around non-SourceTextModules (we now inject a closed over variable API for those via import.meta). I can't imagine any simple usage beyond that in the near term. I could imagine wanting some level of interaction not using public exports such as if you are trying to produce a friendly module system:

let x = '$$$';
export {x} for 'friend';

Mostly I'm just not convinced that the public export API is the same as all important things. Even if we link ahead of time, I think reflection of various things might still be desired.

@kriskowal
Copy link
Member Author

Could you be convinced that would be better served by another method if need arose, e.g., importMeta(specifier)?

@bmeck
Copy link
Member

bmeck commented Oct 22, 2020

@kriskowal probably, but I would likely need to be convinced mostly that we aren't actually needing any integrity guarantees about getting the correct namespace object. Using namespace objects for keys in linking makes me skeptical.

@kriskowal
Copy link
Member Author

I could personally be convinced of an alternate design for module maps that doesn’t communicate linkage to another module using the exotic module exports namespace object as a value (and key in a WeakMap internally to the shim implementation; I would propose a [[Compartment]] internal slot to achieve the same effect).

I can’t imagine a case where the namespace object could be a forgery of confused. Perhaps I can ask you to expand on that worry.

@bmeck
Copy link
Member

bmeck commented Oct 22, 2020

@kriskowal given:

// file: a
export then() {
  return import('b');
}

It would not be possible to get a namespace object of a to put in the graph if the means of obtaining the namespace is through a Promise.

@kriskowal
Copy link
Member Author

Ah, the mechanism for getting a namespace object for the graph is module(spec), which is reliable and doesn’t require pre-initialization of the dependencies, and the behavior of this example would not necessarily be incorrect (and would be consistent with dynamic import, which cannot be changed).

@kriskowal
Copy link
Member Author

It is a very fun example!

@devsnek
Copy link
Member

devsnek commented Oct 22, 2020

If we at least partially adopted #18 it would be pretty trivial to acquire the namespace of modules which export then functions.

@Jack-Works
Copy link
Member

Just a minor note, I prefer "module" to "namespace"

  const { module } = await compartment.import('./main.js');

@Jack-Works
Copy link
Member

This is already removed in the latest proposal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants