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

Question for champions: Minimum viable module descriptors #55

Open
kriskowal opened this issue Jun 10, 2022 · 6 comments
Open

Question for champions: Minimum viable module descriptors #55

kriskowal opened this issue Jun 10, 2022 · 6 comments

Comments

@kriskowal
Copy link
Member

The design of the module descriptor type union unfortunately exists on a sliding scale from minimal to ergonomic.

The original design was: string | namespace, where string was roughly equivalent to the proposed { record: string } and namespace was equivalent to { namespace: ModuleExportsNamespace }. This did not afford us a position to carry importMeta or handle inter-compartment linkage. The SES-shim evolved the API to support an optional “envelope” to handle those cases, and distinguished the envelope from namespace via ModuleExportsNamespace brand check. Our friends at Moddable eventually arrived at the notion of a module descriptor union, which allowed us to simplify this proposal considerably.

I believe the minimal design would be { record: StaticModuleRecord | SyntheticStaticModuleRecord } | { record: string, loader?: Loader } | { instance: string, loader?: Loader }. We can recover { namespace: ModuleExportsNamespace } from { instance: string } and we can recover { namespace: ^ModuleExportsNamespace } with a fake compartment and SyntheticStaticModuleRecord. That is to say, we could remove the namespace descriptors: one or even both.

@patrick-soquet at Moddable is reluctant to include { namespace: ^ModuleExportsNamespace }, which is equivalent to and has the same drawbacks as creating a SyntheticStaticModuleRecord that binds exports for all the own enumerable properties of an arbitrary JavaScript object. The ModuleExportsNamespace inside the comaprtment/loader would be different than the one passed. Live bindings would be limited. That is to say, the form is strictly a convenience.

I believe I can fairly represent @patrick-soquet’s preference that we also keep a string typed descriptor as an abbreviation of { record: string } and a ModuleExportsNamespace abbreviation either in addition to or instead of the equivalent { namespace: ModuleExportsNamespace }.

To that end, I think we should decide on two guiding design principles:

  1. Should we propose the minimum necessary descriptor forms to satisfy collective requirements?
  2. Should we propose a module descriptor type that is strictly a union of descriptor records, or allow shorthands for strings and objects that pass a ModuleExportsNamespace brand check?
@Jack-Works
Copy link
Member

If we're going to remove namespace descriptors, I'd want to require a StaticModuleRecord.ofExports({...})

@kriskowal
Copy link
Member Author

kriskowal commented Jun 10, 2022

Without namespace module descriptors, lifting an arbitrary object into the form of a ModuleExportsNamespace that passes a brand check would be:

const makeExports = object => {
  const names = Object.getOwnPropertyNames(object);
  const record = {
    bindings: names.map(name => ({export: name})),
    initialize(env) {
      for (const name of names) {
        env[name] = object[name];
      }
    },
  };
  const modules = {
    stub: { record },
  };
  const compartment = new Compartment({ modules });
  return compartment.importNow('stub');
}

This has exactly the same caveats as { namespace: NotANamespace }, specifically that it captures a snapshot, changes to the original object are not reflected as live bindings, and that the resulting namespace is not identical to the original object. Apart from that, this “shim” misses an opportunity to cut through a lot of ceremony that, behind the scenes, could just create a namespace. Leaving this to user code has the advantage that the specification does not need to decide which properties to capture from the myriad options: all properties, own properties, or own enumerable properties.

@kriskowal kriskowal changed the title Minimum viable module descriptors Question for champions: Minimum viable module descriptors Jun 14, 2022
@Jamesernator
Copy link

Leaving this to user code has the advantage that the specification does not need to decide which properties to capture from the myriad options: all properties, own properties, or own enumerable properties.

There are pretty standard conventions in the language for this already, operations like Object.entries/{ ...spread }/JSON.stringify (and even future proposals like Record(obj)) all agree on own-enumerable properties.

@ljharb
Copy link
Member

ljharb commented Jun 16, 2022

However, Object.keys/entries/values only include enumerable string property keys; Object.getOwnPropertyNames includes non-enumerable string keys; Reflect.ownKeys includes both non-enumerables and Symbols; Object.assign/object spread includes enumerable strings or Symbols, etc. There's not quite as much agreement as you imply.

@Jamesernator
Copy link

Jamesernator commented Jun 16, 2022

Object.getOwnPropertyNames includes non-enumerable string keys; Reflect.ownKeys includes both non-enumerables and Symbols;

The difference with these methods is they are intentionally specialized towards gettings all own keys, Reflect.ownKeys in particular is meant to reflect precisely what the object contains, it's not there to be opinionated about providing a certain set of keys like Object.keys is. (Also isn't Object.getOwnPropertyNames basically just a pre-symbols version of Reflect.ownKeys?)

The thing about the other methods is they are all higher level in the regard that they could be implemented in userland using the Reflect.* hooks. The fact that they are higher level in this regard, establishes a pretty strong precedent of what a typical object usage would look like. (And indeed this precedent goes beyond ECMA262, given things like structuredClone follow basically the same rules for plain objects).

There are some differences with the various APIs with symbol handling, but like in records that doesn't really apply here because modules don't have symbol keys either.

@kriskowal
Copy link
Member Author

For the record, I observe that, if we included support for { namespace: NotANamespace }, that creating the namespace from the own enumerable string keys would be sufficient and that none among us suggests any other semantics.

That suggests that the crux of this issue lies in whether champions prefer to include or exclude this form in the proposal as a value judgement.

On the one hand, leaving it out reduces the debatable mass of the proposal without reducing what motivating cases are expressible.

Whereas including it invites debate. The other proposed form, { namespace: ActualNamespace }, would behave differently. Actual namespaces can participate with live bindings and the namespace’s identity is preserved inside the compartment module graph.

So the question is whether the champions feel the complexity is worth the cost. So, for a temperature check, I propose meanings for reaction emojis:

  • 🚀 Propose { namespace: ActualNamespace } and { namespace: NotANamespace }
  • ❤️ Propose only { namespace: ActualNamespace }, such that { namespace: makeExports(notANamespace) } from the example above is necessary to lift non-namespaces.

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