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

Get at module export table in native code #3616

Closed
fatcerberus opened this issue Aug 30, 2017 · 23 comments
Closed

Get at module export table in native code #3616

fatcerberus opened this issue Aug 30, 2017 · 23 comments
Assignees

Comments

@fatcerberus
Copy link
Contributor

There doesn't seem to be a way to get at the exports of a module in native code. I'm trying to implement a system where my engine calls an export default function in the root module, but I haven't found a way of accessing the exports. JsModuleEvaluation(moduleRecord, &result); usually just returns undefined for the result.

@fatcerberus
Copy link
Contributor Author

I note also that this limitation prevents me from having require() interoperate with ES modules.

@liminzhu
Copy link
Collaborator

liminzhu commented Sep 1, 2017

There isn't a way to do that today. I'm curious what you intend to do for require.

@fatcerberus
Copy link
Contributor Author

I'm in the process of migrating my miniSphere game engine from Duktape to ChakraCore. Natively, Duktape only fully supports ES5 with a very limited subset of ES6. Because I broadly support ES6 including modules, Babel was an integral part of my toolchain for some time. Unfortunately, Babel transpiles imports to requires, and this plays havoc now that I'm converting my standard library to real ES modules.

I would like to maintain backward compatibility with already-Babelized code if at all possible--without actually using Babel--which leaves me with two options:

  • Leave my standard library in CommonJS format. This is not ideal, as I can't then use them via import. (CJS is dynamic, ESM is static)
  • Come up with a way to allow require() to load the the standard library anyway despite being ESM.

I should note that I've already accomplished the latter by injecting the following shim when a require() requests an ES module: Rather than giving the CommonJS source directly to the module loader (which obviously wouldn't work) I instead send this...

import * as Module from "<specifier>"
global.___exports = Module;

Then I can simply return the value of ___exports from require(). A bit hacky, but it works. However, it would be better if the ChakraCore API could allow this directly and avoid the fake module shim.

@fatcerberus
Copy link
Contributor Author

fatcerberus commented Sep 1, 2017

As far as natively module-based codebases go, without regard for require(): When I was using Babel I was able to have a "main class" exported from the root module that the engine could call on startup, similar to how C# calls Program.Main. Like so:

export default
class Game
{
    start() {
        // initialization
    }
}

With Babel that worked because the module was translated to a CommonJS module, where the class is readily available through the return value of require(). The only way I could maintain compatibility here was to use the above-mentioned shim.

@liminzhu
Copy link
Collaborator

liminzhu commented Sep 4, 2017

+@digitalinfinity

This is very interesting. For us we need to think about how to implement ESM/CJS interop in NodeChakraCore so there will be some changes that can solve the problem you described.

@fatcerberus
Copy link
Contributor Author

Yep, the module implementation will get lots of smoke testing from me because I went all-in with supporting them. :)

By the way does ChakraCore support import()?

@liminzhu
Copy link
Collaborator

liminzhu commented Sep 5, 2017

Yep, dynamic import is in v1.6.0+.

@Oceanswave
Copy link
Contributor

A little confused about this myself.

#3255 seems to have removed returns at the top level (throws on parse module), but at the same time JsModuleEvaluation seems to indicate that it should provide a return value.

However, the return value seems to always be undefined -- even with a export default as @fatcerberus indicates.

is there a mechanism to get a returned or exported value from the top level module?

@fatcerberus
Copy link
Contributor Author

fatcerberus commented Dec 4, 2017

@Oceanswave A hardcoded return value of undefined is correct according to the current specification:
https://tc39.github.io/ecma262/#sec-moduleevaluation

So this would need a new API if such a feature is implemented. A workaround that will work right now is do what I do in miniSphere, instead of evaluating your top-level module directly, you evaluate a shim like this:

import * as Module from "<top-level specifier>"
let global = (new Function('return this;'))();
global.$EXPORTS = Module;

And then you can just read back $EXPORTS to see everything your main module exports.

@boingoing
Copy link
Contributor

So looks like what we want here is a way to fetch the exports of a module from native-side, perhaps only the default export or module namespace object? Something like:

CHAKRA_API
JsGetModuleExport(
    _In_ JsModuleRecord requestModule,
    _In_ JsPropertyIdRef propertyId,
    _Outptr_result_maybenull_ JsValueRef* value);

or

CHAKRA_API
JsGetModuleNamespaceObject(
    _In_ JsModuleRecord requestModule,
    _Outptr_result_maybenull_ JsValueRef* value);

or

CHAKRA_API
JsGetModuleDefaultExport(
    _In_ JsModuleRecord requestModule,
    _Outptr_result_maybenull_ JsValueRef* value);

Does that sound right? Any preference among those options?

@Oceanswave
Copy link
Contributor

@fatcerberus -- thanks for the pointer!

@boingoing -- would be an improvement to have the ability to do that rather than mutate the global object. I like the first option the most but a combo of the first and last would be fine too. Having access to the module namespace would potentially provide more flexibility, but personally not needing that right now.

@fatcerberus
Copy link
Contributor Author

@boingoing My preference is for option 2, just for the sake of future-proofing. Option 3 would be nice to have too, but I’d consider that an auxiliary helper function rather than the primary API.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Feb 7, 2018

@boingoing I’d be interested in making a PR to add one of those APIs if this would be appreciated, probably option 2 to be the most flexible, or a hybrid one that can do any of the 3 with an enumeration parameter thoughts?

@boingoing
Copy link
Contributor

@rhuanjl We'd be happy for the help if you are willing to build a PR adding one of those APIs. I don't think a version which can do any of those with an enumeration parameter makes sense for us. There's no real reason not to just have different API surface and plumb them into the same implementation underneath. If it's up to me, I'd prefer option 2, then 3, then 1 but I am not a consumer of the API so I may be biased. 😀

I will be happy to help if you feel inclined to add this. 👍

@rhuanjl
Copy link
Collaborator

rhuanjl commented Feb 7, 2018

I'll be happy to give this a go after my current API PR #4608 is finished/merged - assuming that this would be wanted.

If you don't like the idea of an enum, I'll probably just make the second option which is the most flexible - my thought behind the enum was that whilst you can do everything with option 2, options 1 and 3 can be easier to use for certain circumstances BUT at the same time adding lots of APIs makes the documentation get bloated hence an enum to have the ease of use of options 1 and 3 when wanted BUT without the 2 extra entries in the documentation.

@boingoing
Copy link
Contributor

I don't think the extra documentation overhead is a big problem for ChakraCore. My main concern with an enum to choose which version of these API you want is what you would do with the property id for option 1. You'd have to make it optional and it would only apply if you passed the enum to activate the 'find export by property name' option which seems a little clunky and hard to explain in the documentation anyway to me. Also, you'll have to add documentation for all the enum values...

/// <summary>
///     Options for JsGetModuleExport
/// </summary>
typedef enum _JsGetModuleExportOption {
  /// <summary>
  ///     Lookup a module export by an identifier name.
  /// </summary>
  JsGetModuleExportOptionNamedProperty = 0x00000000,

  /// <summary>
  ///     Get the module namespace object.
  /// </summary>
  JsGetModuleExportOptionModuleNamespace = 0x00000001,

  /// <summary>
  ///     Get the module default export.
  /// </summary>
  JsGetModuleExportOptionDefaultExport = 0x00000002
} JsGetModuleExportOption;

CHAKRA_API
JsGetModuleExport(
    _In_ JsModuleRecord requestModule,
    _In_ JsGetModuleExportOption option,
    _In_maybenull_ JsPropertyIdRef propertyId,
    _Outptr_result_maybenull_ JsValueRef* value);

@akroshg
Copy link
Contributor

akroshg commented Feb 8, 2018

@boingoing does the module namespace object not have knowledge of which export is default export? If I just get the namespace object I'd be able to enumerate them, no?

@fatcerberus
Copy link
Contributor Author

@akroshg By definition the default export is always called default.

@boingoing
Copy link
Contributor

@akroshg yes, with the module namespace object you should be able to enumerate all the exports including the default.

@liminzhu
Copy link
Collaborator

liminzhu commented Feb 8, 2018

I also like option 2. It's the most basic and versatile and If you have to grab default or a specific export often, it's very easy to write a helper. The enum do-it-all API is too confusing. Can also enumerate exports with JsGetOwnPropertyNames.

@akroshg
Copy link
Contributor

akroshg commented Feb 8, 2018

I like the option 2 as well.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Feb 8, 2018

OK, makes sense - I'll be happy to make a PR for option 2 if this is wanted; though only once 4608 is done (as I assume that running two API PRs at the same time would be a recipe for merge conflicts)

@rhuanjl
Copy link
Collaborator

rhuanjl commented Feb 17, 2018

I'll give this a go now - will likely take me a few days, perhaps a week, my main concern is how I'll I'll build a test for it.

It may be easiest to make a native test - though I don't currently know how your native tests work.

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

No branches or pull requests

7 participants