From 290e084cabee7ccf9facabf49d790531f541a2e4 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 19 Feb 2020 12:25:08 -0500 Subject: [PATCH 1/2] Make optimisticLookupPackageJson return array of package.json objects. Commit 646fa4e3ee6baea585766807bc930123ba4d03f7 fixed #10547 by restricting optimisticLookupPackageJson to package.json files with a "name" property, which effectively skipped over intermediate package.json files with additional properties. However, in Node.js 12.16.0 (Meteor 1.9.1+), modules evaluated natively by Node are considered ECMAScript modules if the closest package.json file has "type": "module" (or has an .mjs file extension). This poses a problem for the module.useNode() trick (see packages/modules-runtime/server.js), because ESM modules cannot be imported using require. For example, recent versions of the @babel/runtime package have a @babel/runtime/helpers/esm/package.json file for the ESM versions of its helpers (which specifies "type": "module"), but that package.json file does not have a "name" property, because it is not the root package.json file representing the entire @babel/runtime package. I considered making the "name" restriction configurable, but that would have fragmented the caching of optimisticLookupPackageJson. Instead, I made it return an array of all potentially relevant package.json objects, which can be safely cached. This means that the caller has to iterate over the array, but there is only one call site for this function (in tools/isobuild/package-source.js) right now, so that wasn't too much work. --- tools/fs/optimistic.ts | 30 ++++++++++++++++++++++++------ tools/isobuild/package-source.js | 14 +++++++++++--- 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/tools/fs/optimistic.ts b/tools/fs/optimistic.ts index 86eb79ee1f..47e64228c7 100644 --- a/tools/fs/optimistic.ts +++ b/tools/fs/optimistic.ts @@ -354,28 +354,46 @@ export const optimisticReadMeteorIgnore = wrap((dir: string) => { type LookupPkgJsonType = OptimisticWrapperFunction< [string, string], - ReturnType + ReturnType[] >; -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) => { diff --git a/tools/isobuild/package-source.js b/tools/isobuild/package-source.js index 15fb77f4a7..27c332c746 100644 --- a/tools/isobuild/package-source.js +++ b/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 { From 87bc54fb9fe0d55251cc0cbdb4066e9103e6c6da Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 19 Feb 2020 12:26:15 -0500 Subject: [PATCH 2/2] Avoid bailing out with module.useNode() for ESM modules. On the server, Meteor attempts to avoid bundling node_modules code by replacing entry point modules with a stub that calls module.useNode() (see packages/modules-runtime/server.js). This trick allows evaluating server node_modules natively in Node.js, faithfully preserving all Node-specific behaviors, such as module.id being an absolute file system path, the __dirname and __filename variables, the ability to import binary .node modules, and so on. However, starting in Node.js 12.16.0 (Meteor 1.9.1+), modules evaluated natively by Node are considered ECMAScript modules (ESM) if the closest package.json file has "type": "module" (or has an .mjs file extension). This poses a problem for the module.useNode() trick, because ESM modules cannot be imported synchronously using require (which is currently how module.useNode() works). To work around this new error, this commit checks package.json for "type": "module" in ImportScanner#shouldUseNode to determine whether it's safe to use the module.useNode() trick. The good news is that ESM modules don't have access to nearly as many Node.js-specific quirks: no module, require, or exports variables; no __dirname, no __filename; no ability to import JSON or other non-ESM file types (at least right now). So it seems somewhat less important for ESM code (compared to CommonJS code) to bail out into native Node.js execution using module.useNode(). In other words, bundling server code should not affect its execution in nearly as many cases, if that code is ESM rather than legacy CommonJS. If this good news turns out to be overly optimistic, we can consider using a different kind of bailout stub that's capable of importing ESM using dynamic import(). For now, making sure we avoid bailing out for ESM code like @babel/runtime/helpers/esm/* is the priority. --- packages/modules-runtime/server.js | 4 +++ tools/isobuild/import-scanner.ts | 50 ++++++++++++++++++++++++++---- 2 files changed, 48 insertions(+), 6 deletions(-) diff --git a/packages/modules-runtime/server.js b/packages/modules-runtime/server.js index 2283500c68..b7da7b880d 100644 --- a/packages/modules-runtime/server.js +++ b/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; diff --git a/tools/isobuild/import-scanner.ts b/tools/isobuild/import-scanner.ts index 12ad54861f..355ad9d0d5 100644 --- a/tools/isobuild/import-scanner.ts +++ b/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