Respond to review feedback

chakra-core · Feb 20, 2018 · 304133a · 304133a
1 parent fd54a47
commit 304133a
Show file tree

Hide file tree

Showing 5 changed files with 51 additions and 31 deletions.
diff --git a/bin/ch/WScriptJsrt.cpp b/bin/ch/WScriptJsrt.cpp
@@ -266,7 +266,7 @@ JsValueRef __stdcall WScriptJsrt::GetModuleNamespace(JsValueRef callee, bool isC
                     errorCode = ChakraRTInterface::JsGetModuleNamespace(moduleEntry->second, &returnValue);
                     if (errorCode == JsErrorModuleNotEvaluated)
                     {
-                        errorMessage = _u("GetModuleNamespace called with un-evaluated module;");
+                        errorMessage = _u("GetModuleNamespace called with un-evaluated module");
                     }
                 }
             }
@@ -278,7 +278,8 @@ JsValueRef __stdcall WScriptJsrt::GetModuleNamespace(JsValueRef callee, bool isC
         JsValueRef errorObject;
         JsValueRef errorMessageString;
 
-        if (wcscmp(errorMessage, _u("")) == 0) {
+        if (wcscmp(errorMessage, _u("")) == 0)
+        {
             errorMessage = ConvertErrorCodeToMessage(errorCode);
         }
 

diff --git a/lib/Jsrt/ChakraCommon.h b/lib/Jsrt/ChakraCommon.h
@@ -225,15 +225,15 @@ typedef unsigned short uint16_t;
         /// </summary>
         JsErrorModuleParsed,
         /// <summary>
-        ///     Module was not yet evaluated when JsGetModuleNamespace was called.
-        /// </summary>
-        JsErrorModuleNotEvaluated,
-        /// <summary>
         ///     Argument passed to JsCreateWeakReference is a primitive that is not managed by the GC.
         ///     No weak reference is required, the value will never be collected.
         /// </summary>
         JsNoWeakRefRequired,
         /// <summary>
+        ///     Module was not yet evaluated when JsGetModuleNamespace was called.
+        /// </summary>
+        JsErrorModuleNotEvaluated,
+        /// <summary>
         ///     Category of errors that relates to errors occurring within the engine itself.
         /// </summary>
         JsErrorCategoryEngine = 0x20000,

diff --git a/lib/Jsrt/Core/JsrtCore.cpp b/lib/Jsrt/Core/JsrtCore.cpp
@@ -227,3 +227,20 @@ JsGetModuleHostInfo(
     });
     return errorCode;
 }
+
+CHAKRA_API JsGetModuleNamespace(_In_ JsModuleRecord requestModule, _Outptr_result_maybenull_ JsValueRef *moduleNamespace)
+{
+    PARAM_NOT_NULL(moduleNamespace);
+    *moduleNamespace = nullptr;
+    if (!Js::SourceTextModuleRecord::Is(requestModule))
+    {
+        return JsErrorInvalidArgument;
+    }
+    Js::SourceTextModuleRecord* moduleRecord = Js::SourceTextModuleRecord::FromHost(requestModule);
+    if (!moduleRecord->WasEvaluated())
+    {
+        return JsErrorModuleNotEvaluated;
+    }
+    *moduleNamespace = (JsValueRef) moduleRecord->GetNamespace();
+    return JsNoError;
+}
diff --git a/lib/Jsrt/Jsrt.cpp b/lib/Jsrt/Jsrt.cpp
@@ -5343,19 +5343,4 @@ CHAKRA_API JsSetHostPromiseRejectionTracker(_In_ JsHostPromiseRejectionTrackerCa
     /*allowInObjectBeforeCollectCallback*/true);
 }
 
-CHAKRA_API JsGetModuleNamespace(_In_ JsModuleRecord requestModule, _Outptr_result_maybenull_ JsValueRef *moduleNamespace)
-{
-    if (!Js::SourceTextModuleRecord::Is(requestModule))
-    {
-        return JsErrorInvalidArgument;
-    }
-    Js::SourceTextModuleRecord* moduleRecord = Js::SourceTextModuleRecord::FromHost(requestModule);
-    if(!moduleRecord->WasEvaluated())
-    {
-        return JsErrorModuleNotEvaluated;
-    }
-    *moduleNamespace = (JsValueRef) moduleRecord->GetNamespace();
-    return JsNoError;
-}
-
 #endif // _CHAKRACOREBUILD
diff --git a/test/es6module/GetModuleNamespace.js b/test/es6module/GetModuleNamespace.js
@@ -40,26 +40,43 @@ WScript.RegisterModuleSource("mod1.js",`
         return await null;
     }
     export const export5 = "exported";
+    export {export6} from "mod2.js";
 `);
 
-let test1 = import("mod0.js").then(()=>{
+WScript.RegisterModuleSource("mod2.js",`
+    export function export6 ()
+    {
+        return true;
+    }
+`);
+
+let test1 = import("mod0.js").then(namespaceFromImport => {
     let mod0Namespace = WScript.GetModuleNamespace("mod0.js");
-    assert.areEqual(mod0Namespace.export1, 5);
-    assert.isTrue(mod0Namespace.default());
-    assert.areEqual(typeof mod0Namespace.export3.constructor, "function");
-    assert.areEqual(mod0Namespace.export4.constructor.name, "AsyncFunction");
-    assert.areEqual(mod0Namespace.export5, "exporting");
+    assert.areEqual(mod0Namespace.export1, 5, "mod0: export1 should equal 5");
+    assert.isTrue(mod0Namespace.default(), "mod0: export2 should return true");
+    assert.areEqual(typeof mod0Namespace.export3.constructor, "function", "mod0: export3 should have constructor function");
+    assert.areEqual(mod0Namespace.export4.constructor.name, "AsyncFunction", "mod0: export4 should be an async function");
+    assert.areEqual(mod0Namespace.export5, "exporting", "mod0: export5 should be a string, exporting");
+    assert.isUndefined(mod0Namespace.notExported1, "Not exported module const should not be property of namespace");
+    assert.isUndefined(mod0Namespace.notExported2, "Not exported module let should not be property of namespace");
+    assert.isUndefined(mod0Namespace.notExported3, "Not exported module var should not be property of namespace");
+    assert.areEqual(namespaceFromImport, mod0Namespace, "ModuleNamespace object should match resolved value of import");
 });
 
-let test2 = import("mod1.js").then(()=>{
+let test2 = import("mod1.js").then(namespaceFromImport => {
     let mod1Namespace = WScript.GetModuleNamespace("mod1.js");
     assert.areEqual(mod1Namespace.export1, 10);
     assert.isFalse(mod1Namespace.default());
+    assert.isUndefined(mod1Namespace.export2, "mod1: Name of default export should be default.")
     assert.areEqual(typeof mod1Namespace.export3.constructor, "function");
     assert.areEqual(mod1Namespace.export4.constructor.name, "AsyncFunction");
+    assert.areEqual(mod1Namespace.export5, "exported");
+    assert.areEqual(mod1Namespace.export6(), true);
+    assert.areEqual(namespaceFromImport, mod1Namespace,"ModuleNamespace object should match resolved value of import");
 });
 
-assert.throws(()=>WScript.GetModuleNamespace("mod0.js"));
-assert.throws(()=>WScript.GetModuleNamespace("mod3.js"));
+assert.throws(()=>WScript.GetModuleNamespace("mod0.js"), Error, "Expected error for un-evaluated module", "GetModuleNamespace called with un-evaluated module");
+assert.throws(()=>WScript.GetModuleNamespace("mod3.js", Error, "Expected error for un-loaded module", "Need to supply a path for an already loaded module for WScript.GetModuleNamespace"));
+assert.throws(()=>WScript.GetModuleNamespace(), Error, "Expected error for no-argument", "Need an argument for WScript.GetModuleNamespace");
 
-Promise.all([test1,test2]).then(()=>print("pass")).catch(()=>print("fail"));
+Promise.all([test1,test2]).then(()=>print("pass")).catch((e)=>print("fail: " + e));