Merge pull request #10926 from meteor/avoid-module.useNode-for-ESM-imports

Avoid bailing out with module.useNode() for ESM modules.

packages/modules-runtime/server.js

@@ -71,6 +71,10 @@ Module.prototype.useNode = function () {
     return false;
   }
 
+  // See tools/static-assets/server/npm-require.js for the implementation
+  // of npmRequire. Note that this strategy fails when importing ESM
+  // modules (typically, a .js file in a package with "type": "module" in
+  // its package.json), as of Node 12.16.0 (Meteor 1.9.1).
   this.exports = npmRequire(this.id);
 
   return true;

tools/fs/optimistic.ts

@@ -354,28 +354,46 @@ export const optimisticReadMeteorIgnore = wrap((dir: string) => {
 
 type LookupPkgJsonType = OptimisticWrapperFunction<
   [string, string],
-  ReturnType<typeof optimisticReadJsonOrNull>
+  ReturnType<typeof optimisticReadJsonOrNull>[]
 >;
 
-export const optimisticLookupPackageJson: LookupPkgJsonType =
+// Returns an array of package.json objects encountered in any directory
+// between relDir and absRootDir. If a package.json with a "name" property
+// is found, it will always be the first object in the array.
+export const optimisticLookupPackageJsonArray: LookupPkgJsonType =
 wrap((absRootDir: string, relDir: string) => {
   const absPkgJsonPath = pathJoin(absRootDir, relDir, "package.json");
   const pkgJson = optimisticReadJsonOrNull(absPkgJsonPath);
+
+  // Named package.json files always terminate the search. This
+  // restriction was first introduced to fix #10547, before this function
+  // was updated to return an array instead of a single object, but
+  // returning here is still an important base case.
   if (pkgJson && typeof pkgJson.name === "string") {
-    return pkgJson;
+    return [pkgJson];
   }
 
   const relParentDir = pathDirname(relDir);
   if (relParentDir === relDir) {
-    return null;
+    return [];
   }
 
   // Stop searching if an ancestor node_modules directory is encountered.
   if (pathBasename(relParentDir) === "node_modules") {
-    return null;
+    return [];
   }
 
-  return optimisticLookupPackageJson(absRootDir, relParentDir);
+  const parentArray =
+    optimisticLookupPackageJsonArray(absRootDir, relParentDir);
+
+  if (pkgJson) {
+    // If an intermediate package.json file was found, add its object to
+    // the array. Since these arrays are cached, we don't want to modify
+    // them using parentArray.push(pkgJson), hence concat.
+    return parentArray.concat(pkgJson);
+  }
+
+  return parentArray;
 });
 
 const optimisticIsSymbolicLink = wrap((path: string) => {

tools/isobuild/import-scanner.ts

@@ -39,6 +39,7 @@ import {
   optimisticStatOrNull,
   optimisticLStatOrNull,
   optimisticHashOrNull,
+  optimisticLookupPackageJsonArray,
 } from "../fs/optimistic";
 
 import { wrap } from "optimism";
@@ -1296,7 +1297,7 @@ export default class ImportScanner {
       // raw version found in node_modules. See also:
       // https://github.com/meteor/meteor-feature-requests/issues/6
 
-    } else if (this.shouldUseNode(absModuleId)) {
+    } else if (this.shouldUseNode(absModuleId, absPath)) {
       // On the server, modules in node_modules directories will be
       // handled natively by Node, so we just need to generate a stub
       // module that calls module.useNode(), rather than calling
@@ -1340,7 +1341,7 @@ export default class ImportScanner {
 
   // Similar to logic in Module.prototype.useNode as defined in
   // packages/modules-runtime/server.js. Introduced to fix issue #10122.
-  private shouldUseNode(absModuleId: string) {
+  private shouldUseNode(absModuleId: string, absPath: string) {
     if (this.isWeb()) {
       // Node should never be used in a browser, obviously.
       return false;
@@ -1360,10 +1361,47 @@ export default class ImportScanner {
       start += 2;
     }
 
-    // If the remaining parts include node_modules, then this is a module
-    // that was installed by npm, and it should be evaluated by Node on
-    // the server.
-    return parts.indexOf("node_modules", start) >= 0;
+    // If the remaining parts do not include node_modules, then this
+    // module was not installed by npm, so we should not try to evaluate
+    // it natively in Node on the server.
+    if (parts.indexOf("node_modules", start) < 0) {
+      return false;
+    }
+
+    // Below this point, we know we're dealing with a module in
+    // node_modules, which means we should try to use module.useNode() to
+    // evaluate the module natively in Node, except if the module is an
+    // ESM module, because then the module cannot be imported using
+    // require (as of Node 12.16.0), so module.useNode() will not work.
+
+    const dotExt = pathExtname(absPath).toLowerCase();
+
+    if (dotExt === ".mjs") {
+      // Although few npm packages actually use .mjs, Node will always
+      // interpret these files as ESM modules, so we can return early.
+      return false;
+    }
+
+    if (dotExt === ".json") {
+      // There's no benefit to using Node to evaluate JSON modules, since
+      // there's nothing Node-specific about the parsing of JSON.
+      return false;
+    }
+
+    if (dotExt === ".js") {
+      const relDir = pathRelative(this.sourceRoot, pathDirname(absPath));
+      const pkgJsonArray =
+        optimisticLookupPackageJsonArray(this.sourceRoot, relDir);
+      // Setting "type":"module" in package.json makes Node treat .js
+      // files within the package as ESM modules.
+      if (pkgJsonArray.some(pkgJson => pkgJson?.type === "module")) {
+        return false;
+      }
+    }
+
+    // Everything else (.node, .wasm, whatever) needs to be handled
+    // natively by Node.
+    return true;
   }
 
   // Returns an absolute module identifier indicating where to install the

tools/isobuild/package-source.js

@@ -34,7 +34,7 @@ import {
   optimisticHashOrNull,
   optimisticStatOrNull,
   optimisticReadMeteorIgnore,
-  optimisticLookupPackageJson,
+  optimisticLookupPackageJsonArray,
 } from "../fs/optimistic";
 
 // XXX: This is a medium-term hack, to avoid having the user set a package name
@@ -1287,8 +1287,16 @@ _.extend(PackageSource.prototype, {
 
       let readOptions = sourceReadOptions;
       if (inNodeModules) {
-        const pkgJson = optimisticLookupPackageJson(self.sourceRoot, dir);
-        if (pkgJson && nodeModulesToRecompile.has(pkgJson.name)) {
+        // This is an array because (in some rare cases) an npm package
+        // may have nested package.json files with additional properties.
+        const pkgJsonArray = optimisticLookupPackageJsonArray(self.sourceRoot, dir);
+
+        // If a package.json file with a "name" property is found, it will
+        // always be the first in the array.
+        const pkgJson = pkgJsonArray[0];
+
+        if (pkgJson && pkgJson.name &&
+            nodeModulesToRecompile.has(pkgJson.name)) {
           // Avoid caching node_modules code recompiled by Meteor.
           cacheKey = false;
         } else {