Rough implementation of module-relative resolution

We would like to allow modules to import from relative paths, e.g.,

    import mod from './mod';

As things stand, module specifiers (the `'./mod'` bit above) are
treated as globally identifying a module, no matter where they are
used. This means:

 - the same module file, imported using different specifiers, loads a
   module twice;
 - same specifier used from different places should refer to different
   modules, but will use the same cached module.

To fix this, we need to map a specifier into something that _does_
properly identify the particular module, from wherever it's
called. The absolute filesystem path (or other plausibly canonical
path) is a good candidate; but as the code stands, there are a couple
of problems:

 1. it's the ModuleResolverCallback (supplied by the client Go code) that
 does the work of finding a module, so that's where the ID has to come
 from.

 2. the ResolverCallback (in C++ code) used by
 Module::InstantiateModule to get the dependencies of a module can't
 have any state associated with it.

To get around these, we have to 1. pass the canonical path back from
the ModuleResolverCallback; and 2. keep track of what each specifier
resolved to, for each module.

Author: squaremo

binding.cc

@@ -39,7 +39,10 @@ struct worker_s {
   std::string last_exception;
   Persistent<Function> recv;
   Persistent<Context> context;
+  // canonical path -> module
   std::map<std::string, Eternal<Module>> modules;
+  // module hash -> specifier -> module
+  std::map<int, std::map<std::string, Eternal<Module>>> resolved;
 };
 
 // Extracts a C string from a V8 Utf8Value.
@@ -61,6 +64,41 @@ void FatalErrorCallback2(const char* location, const char* message) {
 }
 */
 
+MaybeLocal<Module> ResolveCallback(Local<Context> context,
+                                   Local<String> specifier,
+                                   Local<Module> referrer) {
+    auto isolate = Isolate::GetCurrent();
+    worker* w = (worker*)isolate->GetData(0);
+
+    HandleScope handle_scope(isolate);
+
+    String::Utf8Value str(specifier);
+    const char* moduleName = *str;
+
+    if (w->resolved.count(referrer->GetIdentityHash()) == 0) {
+      std::string out;
+      out.append("Module (");
+      out.append(moduleName);
+      out.append(") has not been loaded");
+      out.append("\n");
+      w->last_exception = out;
+      return MaybeLocal<Module>();
+    }
+
+    std::map<std::string, Eternal<Module>> localResolve = w->resolved[referrer->GetIdentityHash()];
+    if (localResolve.count(moduleName) == 0) {
+      std::string out;
+      out.append("Module (");
+      out.append(moduleName);
+      out.append(") has not been loaded");
+      out.append("\n");
+      w->last_exception = out;
+      return MaybeLocal<Module>();
+    }
+
+    return localResolve[moduleName].Get(isolate);
+}
+
 void ExitOnPromiseRejectCallback(PromiseRejectMessage promise_reject_message) {
   auto isolate = Isolate::GetCurrent();
   worker* w = (worker*)isolate->GetData(0);
@@ -93,30 +131,6 @@ void ExitOnPromiseRejectCallback(PromiseRejectMessage promise_reject_message) {
   exit(1);
 }
 
-MaybeLocal<Module> ResolveCallback(Local<Context> context,
-                                   Local<String> specifier,
-                                   Local<Module> referrer) {
-  auto isolate = Isolate::GetCurrent();
-  worker* w = (worker*)isolate->GetData(0);
-
-  HandleScope handle_scope(isolate);
-
-  String::Utf8Value str(specifier);
-  const char* moduleName = *str;
-
-  if (w->modules.count(moduleName) == 0) {
-    std::string out;
-    out.append("Module (");
-    out.append(moduleName);
-    out.append(") has not been loaded");
-    out.append("\n");
-    w->last_exception = out;
-    return MaybeLocal<Module>();
-  }
-
-  return w->modules[moduleName].Get(isolate);
-}
-
 // Exception details will be appended to the first argument.
 std::string ExceptionString(worker* w, TryCatch* try_catch) {
   std::string out;
@@ -224,6 +238,12 @@ int worker_load(worker* w, char* name_s, char* source_s) {
 }
 
 int worker_load_module(worker* w, char* name_s, char* source_s, int callback_index) {
+  // Assuming the script name is canonical, we can return immediately
+  // if has already been loaded.
+  if (w->modules.count(name_s) != 0) {
+    return 0;
+  }
+
   Locker locker(w->isolate);
   Isolate::Scope isolate_scope(w->isolate);
   HandleScope handle_scope(w->isolate);
@@ -257,33 +277,42 @@ int worker_load_module(worker* w, char* name_s, char* source_s, int callback_ind
     return 1;
   }
 
+  // Keep track of the canonical names for resolved modules
+  std::map<std::string, Eternal<Module>> resolved;
+
   for (int i = 0; i < module->GetModuleRequestsLength(); i++) {
     Local<String> dependency = module->GetModuleRequest(i);
     String::Utf8Value str(dependency);
     char* dependencySpecifier = *str;
 
-    // If we've already loaded the module, skip resolving it.
-    // TODO: Is there ever a time when the specifier would be the same
-    // but would need to be resolved again?
-    if (w->modules.count(dependencySpecifier) != 0) {
-      continue;
-    }
-
-    int ret = ResolveModule(dependencySpecifier, name_s, callback_index);
-    if (ret != 0) {
+    struct ResolveModule_return retval = ResolveModule(dependencySpecifier, name_s, callback_index);
+    if (retval.r1 != 0) {
       // TODO: Use module->GetModuleRequestLocation() to get source locations
       std::string out;
       out.append("Module (");
       out.append(dependencySpecifier);
       out.append(") has not been loaded");
       out.append("\n");
       w->last_exception = out;
-      return ret;
+      return retval.r1;
+    }
+    // If we were successful in loading that module, it shall be
+    // present in the modules table.
+    if (w->modules.count(retval.r0) == 0) {
+      std::string out;
+      out.append("Module dependency (");
+      out.append(retval.r0);
+      out.append(") has not been loaded");
+      out.append("\n");
+      w->last_exception = out;
+      return 1;
     }
+    resolved[dependencySpecifier] = w->modules[retval.r0];
   }
 
   Eternal<Module> persModule(w->isolate, module);
   w->modules[name_s] = persModule;
+  w->resolved[module->GetIdentityHash()] = resolved;
 
   Maybe<bool> ok = module->InstantiateModule(context, ResolveCallback);
 

worker.go

@@ -54,7 +54,7 @@ var workerTableNextAvailable workerTableIndex = 0
 type ReceiveMessageCallback func(msg []byte) []byte
 
 // To resolve modules from javascript.
-type ModuleResolverCallback func(moduleName, referrerName string) int
+type ModuleResolverCallback func(moduleName, referrerName string) (string, int)
 
 // Don't init V8 more than once.
 var initV8Once sync.Once
@@ -134,7 +134,7 @@ func recvCb(buf unsafe.Pointer, buflen C.int, index workerTableIndex) C.buf {
 }
 
 //export ResolveModule
-func ResolveModule(moduleSpecifier *C.char, referrerSpecifier *C.char, resolverToken int) C.int {
+func ResolveModule(moduleSpecifier *C.char, referrerSpecifier *C.char, resolverToken int) (*C.char, C.int) {
 	moduleName := C.GoString(moduleSpecifier)
 	// TODO: Remove this when I'm not dealing with Node resolution anymore
 	referrerName := C.GoString(referrerSpecifier)
@@ -144,10 +144,11 @@ func ResolveModule(moduleSpecifier *C.char, referrerSpecifier *C.char, resolverT
 	resolverTableLock.Unlock()
 
 	if resolve == nil {
-		return C.int(1)
+		return nil, C.int(1)
 	}
-	ret := resolve(moduleName, referrerName)
-	return C.int(ret)
+	canon, ret := resolve(moduleName, referrerName)
+
+	return C.CString(canon), C.int(ret)
 }
 
 // Creates a new worker, which corresponds to a V8 isolate. A single threaded

worker_test.go

@@ -226,7 +226,7 @@ func TestModules(t *testing.T) {
 	err2 := worker.LoadModule("code.js", `
 		import { test } from "dependency.js";
 		V8Worker2.print(test);
-	`, func(specifier string, referrer string) int {
+	`, func(specifier string, referrer string) (string, int) {
 		if specifier != "dependency.js" {
 			t.Fatal(`Expected "dependency.js" specifier`)
 		}
@@ -235,14 +235,14 @@ func TestModules(t *testing.T) {
 		}
 		err1 := worker.LoadModule("dependency.js", `
 			export const test = "ready";
-		`, func(_, _ string) int {
+		`, func(_, _ string) (string, int) {
 			t.Fatal(`Expected module resolver callback to not be called`)
-			return 1
+			return "", 1
 		})
 		if err1 != nil {
 			t.Fatal(err1)
 		}
-		return 0
+		return "dependency.js", 0
 	})
 	if err2 != nil {
 		t.Fatal(err2)
@@ -257,11 +257,11 @@ func TestModulesMissingDependency(t *testing.T) {
 	err := worker.LoadModule("code.js", `
 		import { test } from "missing.js";
 		V8Worker2.print(test);
-	`, func(specifier string, referrer string) int {
+	`, func(specifier string, referrer string) (string, int) {
 		if specifier != "missing.js" {
 			t.Fatal(`Expected "missing.js" specifier`)
 		}
-		return 1
+		return "", 1
 	})
 	errorContains(t, err, "missing.js")
 }