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

RFC: Realm.prototype.import #294

Closed
leobalter opened this issue Mar 29, 2021 · 29 comments
Closed

RFC: Realm.prototype.import #294

leobalter opened this issue Mar 29, 2021 · 29 comments

Comments

@leobalter
Copy link
Member

leobalter commented Mar 29, 2021

Reflecting the explainer work from #292, I believe we might need to revive Realm#import, returning a promise that resolves to undefined to flag when the code injection is complete.

This would allow use cases like the tests like:

const r = new Realm();

const runTests = await r.importBinding('test-framework', 'run');
await r.import('./my-tests.js');

runTests(done => console.log(done));

Without this, it seems overkill to require ./my-tests.js to export something because it would run in a new realm. Ideally, code should run seamlessly.

The other option I like, suggested by @littledan, is reusing the Realm#import to set the binding within the options arg, fully replacing importBinding.

const r = new Realm();

const runTests = await r.import('test-framework', { binding: 'run' });
await r.import('./my-tests.js');

runTests(done => console.log(done));

This would match the ergonomics from import assertions.

This creates a new question to investigate if dynamic import should have a binding option regardless of it being in the Realms API.

@leobalter leobalter changed the title Realm.prototype.import RFC: Realm.prototype.import Mar 29, 2021
@leobalter
Copy link
Member Author

cc @caridy

@ljharb
Copy link
Member

ljharb commented Mar 29, 2021

I'm confused, what would that binding option do in regular import()?

@leobalter
Copy link
Member Author

I'm confused, what would that binding option do in regular import()?

in regular import it wouldn't be very useful, but you can just do it to import the binding directly. It isn't very useful outside of the Realm API, just consistent. I'd be happy to discard this option.

const { default: doSomething } = import('./my-code.js');

// Not a big pro, but avoids some destructuring
const doSomething = await import('./my-code', { binding: 'default' });
// Perhaps you could just extra data subset from a json file (smells)
const myData = await import('./data.json', { assert: { type: 'json' }, binding: 'a-subset' });

// equivalent to
const { 'a-subset': myData } = await import('./data.json', { assert: { type: 'json' }});

there is little justification for the dynamic import consistency case, but I still like when this model applies to realms import, but limited to primitives and callable wrappers.

@ljharb
Copy link
Member

ljharb commented Mar 29, 2021

How would this work with a thenable module? Would binding destructure off of the resolved value?

What happens if the resolved value is nullish?

export function then(resolve) {
  resolve(undefined);
}

@leobalter
Copy link
Member Author

I need to refresh my brain on the evaluation of this but in any case, internal unwrapping before await and then is equivalent to HostImportModuleDynamically and I guess you just get the name from the actual imported namespace.

Whoever is requiring a binding, does it explicitly anyway. Let me run some tests.

@ljharb
Copy link
Member

ljharb commented Mar 30, 2021

What I mean tho is, "binding" doesn't really make sense to me with import(), since it provides a Promise for a value, not any bindings.

I can see utility in filtering export names - that way, I could avoid a thenable module exercising that functionality - but that's not really "bindings".

@leobalter
Copy link
Member Author

The name is fully open for bikeshedding. We could use something like name, pickName, pick, filter, etc etc. Do you have any suggestions?

@ljharb
Copy link
Member

ljharb commented Mar 30, 2021

I want to make sure I understand the semantics before suggesting a name :-) is it indeed a way to provide a list of export names, so that the resulting module namespace object I get will only contain a subset?

@leobalter
Copy link
Member Author

yes, for Realms, we can't get a module ns object.

@ljharb
Copy link
Member

ljharb commented Mar 30, 2021

Are you suggesting that code inside a realm that does import * as ns from 'path' would get an ns that is not a frozen exotic Module Namespace Record object?

@leobalter
Copy link
Member Author

code inside a realm should work normally without fingerprints. Realm#import is the piece that can't return a ns because that ns object belong inside the realm, and not within the incubator.

@ljharb
Copy link
Member

ljharb commented Mar 30, 2021

Whoever holds the Realm object should "own" the realm, presumably, so why couldn't they get that ns object?

Considering they could .import() a data-URI that does import * as ns and then export default [ns], for example - ie, there's no way to prevent them from doing that - what's the value to making it slightly harder?

@leobalter
Copy link
Member Author

The whole idea over the new Isolated Realms (#289) is to not transfer objects cross realms to also avoid identity discontinuity. We trigger code injection in the other Realm through Realm#import but we can't have immediate access to object values that we imported.

#292 has an explainer being updated with that, the rendered specs already has that.

How Realm#import would return a ns object?

@ljharb
Copy link
Member

ljharb commented Mar 30, 2021

Clearly I'm missing a bunch of context here, apologies :-)

If there's no way to get an object into, or out of, a realm, then it sounds like realm.import() is more about "cause this module to be imported into the realm", but modules are imported into other modules (or into a Promise), so i'm still a bit confused as to its purpose. If it's basically "evaluate this module as an entrypoint inside the realm" then "import" seems like a confusing name to me.

@leobalter
Copy link
Member Author

There is a lot to discuss about the Isolated Realms, we would probably benefit of having a sync call - maybe tc39 - so I can present the differences.

Generally saying, realm.import(), without a binding name is exactly as you described, loading that module to be imported. At first, we saw little value on it, and then we used the importBinding(specifier, 'name') to capture one of the exported names. This way we can still capture primitive values (including symbols) or callable objects. Quick recap of Isolated Realms: callable objects are auto wrapped into a function, the other realm receives a new function that connects to it.

My initial examples in this thread, shows some little usage for why we could still use realm.import() without a specific binding.

@ljharb
Copy link
Member

ljharb commented Mar 30, 2021

A sync call sounds great; let's coordinate offline for that.

In your initial examples, you can extract a function from the realm - that function could presumably return any possible object that exists inside the realm, unless that function was wrapped so that it could never throw an in-realm exception nor return an in-realm object (which would seem pretty complex and heavy-handed).

@leobalter
Copy link
Member Author

This is all covered in the Isolated Realms API. :)

@mhofman
Copy link
Member

mhofman commented Mar 31, 2021

Regarding the naming, there may be an alternative:

importBindings(specifier:string , listOfExportedNames: string[]): Promise<PrimitiveValueOrCallable[]>

That way the user can chose to import 0, 1 or multiple bindings using the same call. It'd force array destructuring for single imports, but I'm not sure if it's that bad. The resolved array would of course be from the local realm.

How would this work with a thenable module? Would binding destructure off of the resolved value?

That's an interesting point. Currently a syntactic import() will always end up resolving with the result from the then() export since it goes through the promise capability. However my understanding is that here as currently written the named binding would just be plucked from the module namespace, ignoring thenable modules. This might be surprising to module authors that use the thenable trick to present a specific object when imported dynamically, and would think that because this API is async, it would be similar to a dynamic import().

One way to remove the thenable surprise, at the cost of making the API a little more awkward, would be to split module import and binding resolution in separate calls.

importModules(...specifiers: string[]): Promise<void>;
getModuleBindingValue(moduleSpecifier: string, bindingName: string): PrimitiveValueOrCallable;

Using getModuleBindingValue with a specifier for an unknown or not yet loaded module would result in an exception.

@Jack-Works
Copy link
Member

So why not have a wrapper module NS object? It can solves all problems here

@ljharb
Copy link
Member

ljharb commented Mar 31, 2021

@mhofman right but you don't actually import a binding (even for a static, named import). you're importing a value, that might change out from under you (because static import is what makes the binding, not the act of importing) if it's from a non-const static named export.

@mhofman
Copy link
Member

mhofman commented Mar 31, 2021

@ljharb, I'm not sure I follow your point.

Do you mean that regular static import creates a binding, and the value might change, but for the isolated realms API we only have a plain value that won't change? I think that's why I like the separate calls I proposed last, as we can make it very obvious with the right function name that the value is a snapshot at the time of the call. Maybe getModuleExportedValue() would be a better name, as I suppose binding is confusing in the context of isolated realms.

foo.js:

export let bar = 42;

export function inc() {
  bar++;
}

main.js:

const r = new Realm();

await r.importModules('./foo.js');
let bar = r.getModuleExportedValue('./foo.js', 'bar');
const inc = r.getModuleExportedValue('./foo.js', 'inc');
console.log(bar); // 42
inc();
console.log(bar); // 42
bar = r.getModuleExportedValue('./foo.js', 'bar');
console.log(bar); // 43

// with the existing gotcha for callables since a
// new wrapper is created each time
assert(inc !== r.getModuleExportedValue('./foo.js', 'inc'));

@Jack-Works, IMO creating a new module namespace wrapper is too complicated. We'd need to have getters to reflect the possibility of changing values as mentioned above, which would have to throw if the value is not a primitive or callable. It also wouldn't solve the problem of thenable modules: the then() function of such modules returns a promise object, which would cause an exception when wrapped, making thenable modules completely unusable.

@ljharb
Copy link
Member

ljharb commented Mar 31, 2021

@mhofman yes - i'm saying that dynamic import never creates a binding, it only provides a module namespace exotic object value, whose property values might change. If you're doing a non-static import, thus, that's the only value one should expect to get. Being forced to take a snapshot will break valid use cases (for which live bindings exist in the first place).

@mhofman
Copy link
Member

mhofman commented Mar 31, 2021

I'm admittedly not very familiar with some of the history of the Realms API proposal, but my understanding is that the Isolated Realms API's concept as discussed in #289 is to not have complex objects shared directly between realms, but instead enable userland membrane libraries to create proxy objects representing objects from the other realm. Such a library can very well create a proxy to those module namespace exotic objects. What kind of use cases did you have in mind that would be broken by this low level API only allowing for the snapshot of module exports?

@leobalter
Copy link
Member Author

Considering one import() already "caches" the evaluated module, we still have a single module evaluation on:

const r = new Realm();

const value = await r.importBinding('./inside-code.js', 'value');
const other = await r.importBinding('./inside-code.js', 'otherValue'); // "cached"

There are some good outtakes here: importBinding is not a good name as we fetch the values, not the bindings. So I imagine even a quick renaming to importValue helps.

I'd like to merge @mhofman's idea into something a bit more ergonomic. I don't like providing the specifier all the time, I'd rather use the import return. I'm going with something that wrapps the namespace, and it's weird and it seems overkill:

const r = new Realm();

const realmModule = await r.import('./inside-code.js');

// realmModule would have an internal list of the bindings you could "get"
const doSomething = realmModule.get('default');
const foo = realmModule.get('foo');

The pro here is requiring only 1 await to load the import. It resolves the problem of capturing binding values or not. This allows eventual extensions for import(), e.g. import assertions.

The con is defining an interface for this realmModule piece. It doesn't seem like a low level code API as we are aiming for. It reminds me of Map but it should be limited to a .get functionality only.

A conservative approach without require a binding value would be just requiring a call to Realm#import.

const r = new Realm();

const verifyIntegrity = await r.importValue('./installFakeDOM-framework.js', 'verifyIntegrity');
await r.import('./user-code.js');

const result = verifyIntegrity();

I don't often need any resolution for the user-code.

@leobalter
Copy link
Member Author

but instead enable userland membrane libraries to create proxy objects representing objects from the other realm

yes

@Jack-Works
Copy link
Member

IMO creating a new module namespace wrapper is too complicated.

I don't think so, it just an exotic object with a custom [[Get]] (and some related) slot right?

It also wouldn't solve the problem of thenable modules: the then() function of such modules returns a promise object, which would cause an exception when wrapped, making thenable modules completely unusable.

In the old Realms API, this problem is already solved. The API returns Promise<{ value: Module }> instead of Promise<Module>. It also applies to the isolated version.

@leobalter
Copy link
Member Author

leobalter commented Mar 31, 2021

During the SES meeting, it came to my mind the current status quo is also good enough if we layer things. It's a good refresher that Realms has just a low level API. We could layer modules out such as:

module insideCode {
  export { runTests } from 'test-framework';
  import './my-tests.js';
}

const r = new Realm();
const runTests = await r.importValue(insideCode, 'runTests'); // renaming importBinding to importValue

Or without the module blocks:

// ./inside-code.js
export { runTests } from 'test-framework';
import './my-tests.js';
// from the incubator Realm
const r = new Realm();
const runTests = await r.importValue('./inside-code.js', 'runTests');

@littledan
Copy link
Member

It seems like this suggestion hasn't quite caught on. I'm fine with sticking with importValue as the final API.

@leobalter
Copy link
Member Author

@littledan this is correct. Thanks!

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

5 participants