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

importValue: How to import a module with no exports? #364

Open
Jack-Works opened this issue May 26, 2022 · 18 comments
Open

importValue: How to import a module with no exports? #364

Jack-Works opened this issue May 26, 2022 · 18 comments

Comments

@Jack-Works
Copy link
Member

According to ShadowRealm.prototype.importValue, missing exportName is a TypeError, and inside ShadowRealmImportValue, ExportGetter step 6 says, 6. If hasOwn is false, throw a TypeError exception.

This means I cannot import a module into the ShadowRealm with no exports. (Maybe I just want to let that script run in the ShadowRealm, I don't want any value returned.)

@mhofman
Copy link
Member

mhofman commented May 26, 2022

I think this is related to #350 (comment)
Throwing in this case allows for future behavior to be added.

ShadowRealm is a low level API. You likely won't be able to import most modules as-is with the current proposal, but instead have to use a "loader" module. What prevents you from doing that in this case? A dummy module with a primitive const export that has a side effect of importing your target module and optionally do any init? (IMO side effect execution on import is an anti-pattern and rarely warranted)

@nicolo-ribaudo
Copy link
Member

Does importing it like this work?

myShadowRealm.evaluate(`await import("./module-with-no-exports.js")`);

@mhofman
Copy link
Member

mhofman commented May 26, 2022

You can't do "top-level-await" in an eval. You could however manually wrap the promise:

await new Promise((resolve, reject) => {
  myShadowRealm.evaluate(`(callback) => {
    import("./module-with-no-exports.js").then(
      () => { callback(null); }, 
      (err) => { callback(String(err)); },
    );
  })((err) => {
    if (err != null) {
      reject(err);
    } else {
      resolve();
    }
 });
});

@jdalton
Copy link
Member

jdalton commented May 26, 2022

The restriction on having an export caught me by surprise. I would have assumed this to be similar to APIs like Worker's importScripts. We had a question about how to import code into a realm when unsafe-eval is restricted via CSP. I would have thought importValue could do as an alternative, but with this restriction things get more tricky.

The current spec editors note for importValue states:

EDITOR'S NOTE
Extensible web: This is equivalent to dynamic import without having to evaluate a script source, which might not be available (e.g.: when CSP is blocking source evaluation).

This restriction is counter to that note. The name of importValue does suggest a value. I lack context on why it was scoped as such though. If there's workarounds then the restriction should be brought into question as it appears to create artificial barriers for users.

@ljharb
Copy link
Member

ljharb commented May 26, 2022

I agree; this seems like a strange restriction.

@leobalter
Copy link
Member

@jdalton I believe that goes back to @mhofman's comment.

When importValue was initially designed, the intention was to not assume how to import or reproduce the module namespace object. With that in mind, the idea was requiring the second argument to always refer to a given name.

By the time, an explicit undefined argument in the name position ended up being cast as its string representation, we improved that.

We could say the explicit undefined argument refers to a no-name resolution, but that seems weird if we compare it to import(specifier) or a single parameter version of importValue.

I want to eventually expand importValue to cast a static serialization of the module namespace object if only one parameter is given, which also assumes the second parameter is not defined (or implicitly undefined).

Keep in mind importValue is ergonomic to a dynamic import, this is different from importScripts.

I'd be happy if we can also consider a ShadowRealm.prototype.importScripts but I wonder how is that specified only in ECMAScript.

@mhofman
Copy link
Member

mhofman commented May 26, 2022

Technically without requiring eval you could have a trampoline:

loader.js:

export const load = (fullSpecifier, callback) => {
  import(fullSpecifier).then(
      () => { callback(null); }, 
      (err) => { callback(String(err)); },
    );
  });
};

main.js:

const load = await myShadowRealm.importValue('loader.js', 'load')
  .then((rawLoad) => (specifier) => new Promise((resolve, reject) => {
    const fullSpecifier = new URL(specifier, import.meta.url).href;
    rawLoad(fullSpecifier, (err) => {
      if (err != null) {
        reject(err);
      } else {
        resolve();
      }
   });
  });

await load('./module-with-no-exports.js');

I understand this is not ergonomic, but the only limitation is to be able to load the trampoline, which wouldn't be evaluated.

@caridy
Copy link
Collaborator

caridy commented May 27, 2022

Maybe we should pull the trigger for "Wrapped Module Namespace Exotic Objects" instead. That will allow you to just do realm.import('foo') which will return a promise on the incubator realm that will give you access to things from the underlaying namespace object but following the wrapping semantics.

@littledan
Copy link
Member

I am confused by this thread. importValue is a specialized API which only works for modules which were written with it in mind--there is more that you need to get right besides having an export. As others have mentioned, it is possible to achieve this behavior with either a wrapping module or wrapping code in eval. I would rather hold off on adding this sort of convenience until we have a broader story for how modules which weren't written with ShadowRealm in mind should be usable (e.g. a built-in membrane framework).

@ljharb
Copy link
Member

ljharb commented Jun 2, 2022

If that's the case, then it seems like a subpar API - modules shouldn't have to know about ShadowRealms to be usable inside one.

@mhofman
Copy link
Member

mhofman commented Jun 2, 2022

modules shouldn't have to know about ShadowRealms to be usable inside one.

They don't need to know. However the user of the ShadowRealm API should know that their entrypoint module needs to be compatible with the ShadowRealm API. None of the other modules that entrypoint references need to have that constraint.

@ljharb
Copy link
Member

ljharb commented Jun 2, 2022

@mhofman good clarification, but that still seems problematic to me - i should be able to use any module with a shadowrealm, just like i can use any module with import().

@mhofman
Copy link
Member

mhofman commented Jun 2, 2022

i should be able to use any module with a shadowrealm, just like i can use any module with import().

I think we'll just need to disagree here. shadowRealm.importValue() is not equivalent to import() and I don't think it needs to be,

@littledan
Copy link
Member

littledan commented Jun 6, 2022

We had a pretty extensive discussion about the importValue API in GitHub and TC39 plenary, including @ljharb --it's always been an API for importing a module built for it, just like passing functions over the ShadowRealm boundary is also for functions built for it (in the same way). I'm surprised by this new proposed requirement on the API shape. It seems like the kind of thing that should've been raised before Stage 3; I'd rather not make big changes on the shape of the API now (and I have no idea what these changes would look like).

@ljharb
Copy link
Member

ljharb commented Jun 6, 2022

I'm confused at your phrasing - I don't think anyone proposed a requirement, nor am I suggesting changes. New information can be recognized at any stage, and I hadn't thought about it in these terms prior.

All normative changes are "supposed" to be made before stage 3, but this proposal has made some and is considering others, and other proposals have made many. The process isn't perfect, as we all know.

@littledan
Copy link
Member

What's the new information here?

@ljharb
Copy link
Member

ljharb commented Jun 6, 2022

I newly realized that this means that the module consumer has to know details about the module's authoring to be able to consume it inside a ShadowRealm. Philosophically, I don't think module consumers should ever have to know anything about a module except what kind of values it exports, under which bindings.

@leobalter
Copy link
Member

I plan to discuss this in July so we can dedicate time for this topic along the other things we are already discussing this week at TC39.

I'd be very happy to have an import() equivalent but this brings a lot of design questions. I believe the resolved value should be a simple/ordinary clone of the given module namespace object that is non-dynamic.

We are aware of expressed objections for adopting Structured Serialization across ShadowRealms. Those are still current. I'm also predicting any sort of cloning for the module ns object will emerge the requirement from other delegates to adopt Structured Serialization, so we should expect blockers on both ways.

The current importValue might not be as powerful compared to a import method, but it resolves our requirements for now.

Perhaps we can understand that ShadowRealm.prototype.import can be part of the commitment from the champions of this proposal, but I hope it comes as a separate proposal in which I'm happy to start and present in the next meeting. My goal is to not block the current implementation, as this will have a severe impact on us and the web.

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

Successfully merging a pull request may close this issue.

8 participants