Implement JsGetModuleNamespace API fixes #3616 #4707

rhuanjl · 2018-02-19T23:41:35Z

Implement a new Jsrt API to get a Module Namespace from an evaluated module record. As discussed in #3616

This is intended for use by any embedder who wishes to access module exports from native code e.g. to use one as an entry point - I note that node's implementation of ESModules with v8 uses an equivalent v8 facility and so this is likely one of the required steps towards implementing ESModules in node-chakracore.

For testing purposes only a WScript method has been added to ch that wraps this method to allow fetching a namespace object from within Javascript - this has then been used to create a test case.

cc @liminzhu @boingoing

jackhorton

Another really valuable contribution!

@kfarnung since you just backported one change to 1.9 for use in Node, perhaps this should target 1.9 too? Also note the email I sent about Intl targeting master, so we may have to reconsider the release/branch structure for node in the near future.

boingoing

This is really nice, thank you for taking the time to tackle this one. We need to take the changes to move the code to JsrtCore.cpp, check for nullptr, etc. Overall, though, this looks very good and I don't think we'll have much trouble taking it.

@jackhorton already mentioned it but, yes, we should probably rebase this onto release/1.9. There is discussion internally about how to configure code going from ChakraCore into Node-ChakraCore but this change is stand-alone enough that it can probably land in release/1.9 and avoid that discussion entirely.

boingoing · 2018-02-20T08:54:01Z

lib/Jsrt/ChakraCommon.h

@@ -225,6 +225,10 @@ typedef unsigned short uint16_t;
        /// </summary>
        JsErrorModuleParsed,
        /// <summary>
+        ///     Module was not yet evaluated when JsGetModuleNamespace was called.
+        /// </summary>
+        JsErrorModuleNotEvaluated,


I don't know if we'll be able to add this error code here since this enum is part of the Windows API surface. I'm not actually sure how fixed our enum values are, though. Might be safe if the new one is added at the end. Maybe @curtisman could answer?

Since this is just an enum and not an enum class, I believe the compiler simply aliases it to an integer type in both C and C++. So it should remain both source- and binary-compatible as long as the new constant is added to the end. Adding it to the middle as done here is indeed problematic as it causes everything after it to be renumbered.

Correct. Adding it at the end will solve the problem

I've moved it to the end of the section it's in - as it's the section for incorrect usage of API e.g. wrong parameters - I believe the numbering is restarted for each section (as the section headings have explicit values so this should be ok?

Seems like this location works. Our new code is using a value which was previously unused and no other values in the enum are affected. I think @curtisman was implying to add to the end of the enum in his comment but if he's onboard with this we should be good.

I didn't notice the start of the next section with an explicit value.
Yep. Where you have now at the end of the section is good. Thanks.

boingoing · 2018-02-20T09:09:13Z

lib/Jsrt/Jsrt.cpp

    /*allowInObjectBeforeCollectCallback*/true);
 }

+CHAKRA_API JsGetModuleNamespace(_In_ JsModuleRecord requestModule, _Outptr_result_maybenull_ JsValueRef *moduleNamespace)


I believe this API should live in JsrtCore.cpp instead of Jsrt.cpp. If it's here it needs to be surrounded by #ifdef _CHAKRACOREBUILD but we could avoid that entirely if it's just moved over into that other file where the other module API live anyway.

It was already inside #ifdef _CHAKRACOREBUILD but now moved to JsrtCore.cpp instead.

Sorry, I didn't see the _CHAKRACOREBUILD macro but suppose it must be there because of the preceding core-only API. Thanks for moving it, anyway, I think locating this API near the other module ones is a good idea.

boingoing · 2018-02-20T09:13:27Z

lib/Jsrt/Jsrt.cpp

+        return JsErrorInvalidArgument;
+    }
+    Js::SourceTextModuleRecord* moduleRecord = Js::SourceTextModuleRecord::FromHost(requestModule);
+    if(!moduleRecord->WasEvaluated())


Nitpick: space between if and (

boingoing · 2018-02-20T09:20:02Z

lib/Jsrt/Jsrt.cpp

+    {
+        return JsErrorModuleNotEvaluated;
+    }
+    *moduleNamespace = (JsValueRef) moduleRecord->GetNamespace();


Before dereferencing moduleNamespace make sure it's not null. PARAM_NOT_NULL(moduleNamespace);

Also, this is an _Out_ parameter so you should initialize it (*moduleNamespace = nullptr;) at the beginning of this function after verifying it is not null. We should do this before returning via other paths so that this variable is always initialized.

boingoing · 2018-02-20T09:33:18Z

bin/ch/WScriptJsrt.cpp

+        JsValueRef errorObject;
+        JsValueRef errorMessageString;
+
+        if (wcscmp(errorMessage, _u("")) == 0) {


Nitpick: { character at end of this line should be on new line below.

boingoing · 2018-02-20T10:04:14Z

test/es6module/GetModuleNamespace.js

+    assert.areEqual(mod0Namespace.export5, "exporting");
+});
+
+let test2 = import("mod1.js").then(()=>{


Does the dynamic import statement return a promise resolved with the module namespace object? If it does you could pass it to the then handler and assert that it's strictly equal to the result of GetModuleNamespace, right?

import("mod0.js").then(ns => { //… assert.areEqual(ns, mod0Namespace); }

boingoing · 2018-02-20T10:06:17Z

test/es6module/GetModuleNamespace.js

+});
+
+assert.throws(()=>WScript.GetModuleNamespace("mod0.js"));
+assert.throws(()=>WScript.GetModuleNamespace("mod3.js"));


Add the error case where GetModuleNamespace is called with no arguments.

boingoing · 2018-02-20T10:07:22Z

test/es6module/GetModuleNamespace.js

+    assert.areEqual(mod1Namespace.export4.constructor.name, "AsyncFunction");
+});
+
+assert.throws(()=>WScript.GetModuleNamespace("mod0.js"));


Is it possible to add a test for the JsErrorModuleNotEvaluated case explicitly? Does this line reliably test that?

rhuanjl · 2018-02-20T18:34:35Z

@boingoing I think I've addressed everything other than rebasing to 1.9.

I've commented above on a couple of points.

I think my test cases may be excessive/over the top I was trying to test that every type of export works BUT considering I'm also now testing against the result of import() as long as import() isn't broken half the cases are irrelevant. (That said it uses the same internal mechanism as import()...)

I assume the best way to rebase to 1.9 would be to use a whole new branch and hence a new PR? If so probably best to agree the shape of the commit/pass reviews here now we've started then I can cherrypick to a new branch and open a new PR for the final test/merge.

kfarnung · 2018-02-20T19:00:41Z

@rhuanjl Just leave it pointing to master. We have other changes in master that we'll eventually want in node-chakracore so we'll have to cut a new branch anyway.

sethbrenith · 2018-02-20T19:04:21Z

lib/Jsrt/Core/JsrtCore.cpp

+    {
+        return JsErrorModuleNotEvaluated;
+    }
+    *moduleNamespace = (JsValueRef) moduleRecord->GetNamespace();


(JsValueRef) [](start = 23, length = 12)

I prefer explicit casting operators like static_cast or reinterpret_cast because their behavior is much clearer than C-style casts.

boingoing

LGTM, thanks for addressing my feedback.

sethbrenith

liminzhu · 2018-02-20T19:44:38Z

LGTM thanks @rhuanjl !

boingoing · 2018-02-20T20:00:35Z

lib/Jsrt/Core/JsrtCore.cpp

+    {
+        return JsErrorModuleNotEvaluated;
+    }
+    *moduleNamespace = static_cast <JsValueRef>(moduleRecord->GetNamespace());


Tiny nitpick: don't put a space between static_cast and the <

rhuanjl · 2018-02-20T20:06:44Z

OK I think everything's done, let me know if you do want it rebased to 1.9, I've left it on master for now as per the comment from @kfarnung.

I note that I think modules in node-chakracore will take quite a lot of work on the chakra-shim side in addition to this - so probably no reason to fast track this to node-chakracore as it won't be able to use it without significant additional effort.

boingoing · 2018-02-20T21:37:46Z

@rhuanjl We'll continue on and merge this to master, don't worry about porting it to release/1.9. Sorry about the confusion.

Thanks for another great contribution, I'll kick off the internal testing and try to marshal it through.

@liminzhu

Merge pull request #4707 from rhuanjl:moduleNamespace Implement a new Jsrt API to get a Module Namespace from an evaluated module record. As discussed in #3616 This is intended for use by any embedder who wishes to access module exports from native code e.g. to use one as an entry point - I note that node's implementation of ESModules with v8 uses an equivalent v8 facility and so this is likely one of the required steps towards implementing ESModules in node-chakracore. For testing purposes only a WScript method has been added to ch that wraps this method to allow fetching a namespace object from within Javascript - this has then been used to create a test case. *cc* @liminzhu @boingoing

rhuanjl force-pushed the moduleNamespace branch 3 times, most recently from e2a68c3 to fd54a47 Compare February 20, 2018 00:14

jackhorton approved these changes Feb 20, 2018

View reviewed changes

boingoing suggested changes Feb 20, 2018

View reviewed changes

boingoing self-assigned this Feb 20, 2018

sethbrenith reviewed Feb 20, 2018

View reviewed changes

boingoing approved these changes Feb 20, 2018

View reviewed changes

sethbrenith approved these changes Feb 20, 2018

View reviewed changes

kfarnung approved these changes Feb 20, 2018

View reviewed changes

rhuanjl force-pushed the moduleNamespace branch from 304133a to 8d58dc2 Compare February 20, 2018 19:46

boingoing reviewed Feb 20, 2018

View reviewed changes

Implement JsGetModuleNamespace API in Jsrt and ch

ee9daa6

rhuanjl force-pushed the moduleNamespace branch from 8d58dc2 to ee9daa6 Compare February 20, 2018 20:02

chakrabot merged commit ee9daa6 into chakra-core:master Feb 20, 2018

rhuanjl deleted the moduleNamespace branch February 20, 2018 22:31

Oceanswave mentioned this pull request Mar 1, 2018

Improve module evaluation by not requiring shim scripts in order to access namespace objects. BaristaLabs/BaristaCore#57

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement JsGetModuleNamespace API fixes #3616 #4707

Implement JsGetModuleNamespace API fixes #3616 #4707

rhuanjl commented Feb 19, 2018 •

edited

Loading

jackhorton left a comment

boingoing left a comment

boingoing Feb 20, 2018

fatcerberus Feb 20, 2018

curtisman Feb 20, 2018

rhuanjl Feb 20, 2018

boingoing Feb 20, 2018

curtisman Feb 20, 2018

boingoing Feb 20, 2018

rhuanjl Feb 20, 2018

boingoing Feb 20, 2018

boingoing Feb 20, 2018

boingoing Feb 20, 2018

boingoing Feb 20, 2018

boingoing Feb 20, 2018

boingoing Feb 20, 2018

boingoing Feb 20, 2018

boingoing Feb 20, 2018

rhuanjl commented Feb 20, 2018 •

edited

Loading

kfarnung commented Feb 20, 2018

sethbrenith Feb 20, 2018

boingoing left a comment

sethbrenith left a comment

liminzhu commented Feb 20, 2018

boingoing Feb 20, 2018

rhuanjl commented Feb 20, 2018 •

edited

Loading

boingoing commented Feb 20, 2018

Implement JsGetModuleNamespace API fixes #3616 #4707

Implement JsGetModuleNamespace API fixes #3616 #4707

Conversation

rhuanjl commented Feb 19, 2018 • edited Loading

jackhorton left a comment

Choose a reason for hiding this comment

boingoing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuanjl commented Feb 20, 2018 • edited Loading

kfarnung commented Feb 20, 2018

Choose a reason for hiding this comment

boingoing left a comment

Choose a reason for hiding this comment

sethbrenith left a comment

Choose a reason for hiding this comment

liminzhu commented Feb 20, 2018

Choose a reason for hiding this comment

rhuanjl commented Feb 20, 2018 • edited Loading

boingoing commented Feb 20, 2018

rhuanjl commented Feb 19, 2018 •

edited

Loading

rhuanjl commented Feb 20, 2018 •

edited

Loading

rhuanjl commented Feb 20, 2018 •

edited

Loading