From 72c704bcaebea393f7cef2a7ece98b4be2094692 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Sun, 5 May 2019 11:32:12 -0400 Subject: [PATCH 1/2] Revert using _findSources to scan .npm/package/node_modules. After much thought, I believe this implementation (#10545) would have caused severe compatibility problems when using packages published with earlier versions of Meteor in a Meteor 1.8.2 app, or when publishing packages with Meteor 1.8.2 for use with earlier Meteor versions. Specifically, this implementation relied on writing the additional .npm/package/node_modules resources found by _findSources into the unibuild JSON file(s), and there just wasn't any good way to make sure the new JSON format could be safely consumed by previous Meteor versions. Even if we found a way to hide the new resources from older versions of Meteor, perhaps by putting them in a new/different property of the unibuild JSON file, packages published with older Meteor versions might try to load an npm package with a "module" field without realizing the code must be compiled, which would likely cause a syntax error in Meteor 1.8.2, since the "module" field always gets preference over the "main" field of package.json (in Meteor 1.8.2). --- tools/isobuild/import-scanner.js | 21 --------------------- tools/isobuild/package-source.js | 22 +--------------------- tools/isobuild/unibuild.js | 32 ++------------------------------ 3 files changed, 3 insertions(+), 72 deletions(-) diff --git a/tools/isobuild/import-scanner.js b/tools/isobuild/import-scanner.js index b90a175fe4..6e4b033449 100644 --- a/tools/isobuild/import-scanner.js +++ b/tools/isobuild/import-scanner.js @@ -375,27 +375,6 @@ export default class ImportScanner { _checkSourceAndTargetPaths(file) { file.sourcePath = this._getSourcePath(file); - // If we're scanning a Meteor package (as indicated by this.name), and we - // come across a file whose sourcePath starts with .npm/, it's a file that - // was installed by Npm.depends, so we should reroot it relative to one of - // this.nodeModulesPaths, rather than preserving the .npm/ path. - if (this.name && file.sourcePath.startsWith(".npm/")) { - const parts = file.sourcePath.split("/"); - const nmi = parts.indexOf("node_modules"); - if (nmi >= 0) { - const suffix = parts.slice(nmi + 1).join("/"); - this.nodeModulesPaths.some(nodeModulesPath => { - const newAbsPath = pathJoin(nodeModulesPath, suffix); - if (optimisticStatOrNull(newAbsPath)) { - file.sourcePath = file.targetPath = - pathRelative(this.sourceRoot, newAbsPath); - return true; - } - return false; - }); - } - } - if (! isString(file.targetPath)) { return; } diff --git a/tools/isobuild/package-source.js b/tools/isobuild/package-source.js index 9625d876a6..210f6d03e8 100644 --- a/tools/isobuild/package-source.js +++ b/tools/isobuild/package-source.js @@ -1343,27 +1343,7 @@ _.extend(PackageSource.prototype, { return sources; } - const sources = find("", 0, false); - - if (!isApp && typeof this.npmCacheDirectory === "string") { - // If this PackageSource has an npmCacheDirectory, scan it as well for - // sources that might need to be compiled. - const stat = optimisticStatOrNull(this.npmCacheDirectory); - if (stat && stat.isDirectory()) { - const relNpmDir = files.pathRelative( - this.sourceRoot, - this.npmCacheDirectory, - ); - if (! relNpmDir.startsWith("..")) { - const relParts = relNpmDir.split("/"); - const depth = relParts.length; - const inNodeModules = relParts.indexOf("node_modules") >= 0; - sources.push(...find(relNpmDir, depth, inNodeModules)); - } - } - } - - return sources; + return files.withCache(() => find("", 0, false)); }), _findAssets({ diff --git a/tools/isobuild/unibuild.js b/tools/isobuild/unibuild.js index 26c09e03e4..d3bc27a1a4 100644 --- a/tools/isobuild/unibuild.js +++ b/tools/isobuild/unibuild.js @@ -251,7 +251,6 @@ export class Unibuild { // Figure out where the npm dependencies go. let node_modules = {}; - let nonLocalNodeModulesBundlePath; _.each(unibuild.nodeModulesDirectories, nmd => { const bundlePath = _.has(npmDirsToCopy, nmd.sourcePath) // We already have this npm directory from another unibuild. @@ -259,9 +258,6 @@ export class Unibuild { : npmDirsToCopy[nmd.sourcePath] = nmd.getPreferredBundlePath("isopack"); node_modules[bundlePath] = nmd.toJSON(); - if (!nmd.local) { - nonLocalNodeModulesBundlePath = nonLocalNodeModulesBundlePath || bundlePath; - } }); const preferredPaths = Object.keys(node_modules); @@ -319,37 +315,13 @@ export class Unibuild { }); // Output other resources each to their own file - _.each(unibuild.resources, resource => { + _.each(unibuild.resources, function (resource) { if (_.contains(["head", "body"], resource.type)) { // already did this one return; } - let generatedFilename; - - // Although non-local npm dependencies installed by Npm.depends start - // with relative paths like .npm/**/node_modules/*, the files are stored - // in the bundle under npm/node_modules, so we need to reroot relative - // .npm/ paths against the npm/node_modules path. - if ( - unibuild.pkg.name && - typeof nonLocalNodeModulesBundlePath === "string" && - typeof resource.path === "string" && - resource.path.startsWith(".npm/") - ) { - const parts = resource.path.split("/"); - const nmi = parts.indexOf("node_modules"); - if (nmi >= 0) { - // Skip builder.writeToGeneratedFilename below since the file already - // (should) exist at this new generatedFilename path. - generatedFilename = files.pathJoin( - nonLocalNodeModulesBundlePath, - parts.slice(nmi + 1).join("/"), - ); - } - } - - generatedFilename = generatedFilename || + const generatedFilename = builder.writeToGeneratedFilename( files.pathJoin( unibuildDir, From f4af3ab2fa02fbc14d70d499087b67ba91a97c1e Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Sat, 4 May 2019 20:45:03 -0400 Subject: [PATCH 2/2] Backstop ESM module compilation with Reify in the ImportScanner. Instead of compiling ESM syntax in node_modules using compiler plugins, the ImportScanner can provide "native" support for ESM syntax by using Reify to quickly compile just the import/export syntax in any imported modules that were not already handled by compiler plugins. Since this code runs every time the app is built, it should not matter which version of Meteor was used to publish a package. Compared to the previous implementation based on PackageSource#_findSources and unibuild JSON files (#10545), this implementation should have far fewer compatibility concerns, as well as being faster thanks to not processing or compiling modules until the ImportScanner determines that they are actually imported. Though the number of files that get compiled by this system should be relatively small for now, to maintain good performance, the results of the compilation are cached on disk and in memory. --- tools/fs/optimistic.js | 6 ++ tools/isobuild/bundler.js | 11 ++- tools/isobuild/compiler-plugin.js | 16 +++- tools/isobuild/import-scanner.js | 153 ++++++++++++++++++++++-------- 4 files changed, 144 insertions(+), 42 deletions(-) diff --git a/tools/fs/optimistic.js b/tools/fs/optimistic.js index 578479b4f0..b65776367f 100644 --- a/tools/fs/optimistic.js +++ b/tools/fs/optimistic.js @@ -5,6 +5,7 @@ import { watch } from "./safe-watcher.js"; import { sha1 } from "./watch.js"; import { pathSep, + pathBasename, pathDirname, pathIsAbsolute, pathJoin, @@ -287,6 +288,11 @@ export const optimisticLookupPackageJson = wrap((absRootDir, relDir) => { return null; } + // Stop searching if an ancestor node_modules directory is encountered. + if (pathBasename(relParentDir) === "node_modules") { + return null; + } + return optimisticLookupPackageJson(absRootDir, relParentDir); }); diff --git a/tools/isobuild/bundler.js b/tools/isobuild/bundler.js index 267e1ee60b..1f7fd85365 100644 --- a/tools/isobuild/bundler.js +++ b/tools/isobuild/bundler.js @@ -1078,13 +1078,20 @@ class Target { } const target = this; + + const linkerCacheDir = this.bundlerCacheDir && + files.pathJoin(this.bundlerCacheDir, "linker"); + + const scannerCacheDir = this.bundlerCacheDir && + files.pathJoin(this.bundlerCacheDir, "scanner"); + const processor = new compilerPluginModule.CompilerPluginProcessor({ unibuilds: this.unibuilds, arch: this.arch, sourceRoot: this.sourceRoot, isopackCache: this.isopackCache, - linkerCacheDir: this.bundlerCacheDir && - files.pathJoin(this.bundlerCacheDir, 'linker'), + linkerCacheDir, + scannerCacheDir, // Takes a CssOutputResource and returns a string of minified CSS, // or null to indicate no minification occurred. diff --git a/tools/isobuild/compiler-plugin.js b/tools/isobuild/compiler-plugin.js index 726cb34f4d..a9ae6d485f 100644 --- a/tools/isobuild/compiler-plugin.js +++ b/tools/isobuild/compiler-plugin.js @@ -110,6 +110,7 @@ export class CompilerPluginProcessor { sourceRoot, isopackCache, linkerCacheDir, + scannerCacheDir, minifyCssResource, }) { Object.assign(this, { @@ -118,11 +119,16 @@ export class CompilerPluginProcessor { sourceRoot, isopackCache, linkerCacheDir, + scannerCacheDir, minifyCssResource, }); - if (this.linkerCacheDir) { - files.mkdir_p(this.linkerCacheDir); + if (linkerCacheDir) { + files.mkdir_p(linkerCacheDir); + } + + if (scannerCacheDir) { + files.mkdir_p(scannerCacheDir); } } @@ -141,7 +147,8 @@ export class CompilerPluginProcessor { return new PackageSourceBatch(unibuild, self, { sourceRoot, - linkerCacheDir: self.linkerCacheDir + linkerCacheDir: self.linkerCacheDir, + scannerCacheDir: self.scannerCacheDir, }); }); @@ -1036,6 +1043,7 @@ export class PackageSourceBatch { constructor(unibuild, processor, { sourceRoot, linkerCacheDir, + scannerCacheDir, }) { const self = this; buildmessage.assertInJob(); @@ -1044,6 +1052,7 @@ export class PackageSourceBatch { self.processor = processor; self.sourceRoot = sourceRoot; self.linkerCacheDir = linkerCacheDir; + self.scannerCacheDir = scannerCacheDir; self.importExtensions = [".js", ".json"]; self._nodeModulesPaths = null; @@ -1293,6 +1302,7 @@ export class PackageSourceBatch { sourceRoot: batch.sourceRoot, nodeModulesPaths, watchSet: batch.unibuild.watchSet, + cacheDir: batch.scannerCacheDir, }); scanner.addInputFiles(map.get(name).files); diff --git a/tools/isobuild/import-scanner.js b/tools/isobuild/import-scanner.js index 6e4b033449..37d7c9a31b 100644 --- a/tools/isobuild/import-scanner.js +++ b/tools/isobuild/import-scanner.js @@ -23,6 +23,7 @@ import { convertToOSPath, convertToPosixPath, realpathOrNull, + writeFileAtomically, } from "../fs/files.js"; const { @@ -34,11 +35,15 @@ const { import { optimisticReadFile, optimisticStatOrNull, + optimisticLookupPackageJson, optimisticLStatOrNull, optimisticHashOrNull, shouldWatch, } from "../fs/optimistic.js"; +import { wrap } from "optimism"; +import { compile as reifyCompile } from "reify/lib/compiler"; + import Resolver from "./resolver.js"; const fakeFileStat = { @@ -55,24 +60,91 @@ const fakeFileStat = { // to prevent them from being added to scanner.outputFiles. const fakeSymbol = Symbol("fake"); -// Default handlers for well-known file extensions. -// Note that these function expect strings, not Buffer objects. -const defaultExtensionHandlers = { - ".js"(dataString) { - // Strip any #! line from the beginning of the file. - return dataString.replace(/^#![^\n]*/, ""); - }, +function stripHashBang(dataString) { + return dataString.replace(/^#![^\n]*/, ""); +} - ".json"(dataString) { - const file = this; - file.jsonData = JSON.parse(dataString); +const reifyCompileWithCache = wrap(function ({ dataString }) { + return reifyCompile(stripHashBang(dataString)).code; +}, { + makeCacheKey({ hash }) { + return hash; + } +}); + +class DefaultHandlers { + constructor({ + bundleArch, + sourceRoot, + cacheDir, + }) { + Object.assign(this, { + isWeb: ! archMatches(bundleArch, "os"), + sourceRoot, + cacheDir, + }); + } + + lookupPackageJson(file) { + const relDir = pathRelative( + this.sourceRoot, + pathDirname(file.absPath), + ); + + if (relDir.startsWith("..")) { + const absParts = file.absPath.split("/"); + absParts.pop(); // Get rid of base filename. + const nmi = absParts.lastIndexOf("node_modules"); + return optimisticLookupPackageJson( + absParts.slice(0, nmi + 1).join("/"), + absParts.slice(nmi + 1).join("/"), + ); + } + + return optimisticLookupPackageJson(this.sourceRoot, relDir); + } + + js(file) { + const pkgJson = this.lookupPackageJson(file); + + // Similar to logic in PackageSource#_findSources. + const hasModuleEntryPoint = pkgJson && ( + this.isWeb + ? typeof pkgJson.module === "string" + : pkgJson.type === "module" + ); + + if (! hasModuleEntryPoint) { + return stripHashBang(file.dataString); + } + + if (this.cacheDir) { + const cacheFileName = pathJoin( + this.cacheDir, + "reify-" + file.hash + ".js", + ); + try { + return optimisticReadFile(cacheFileName, "utf8"); + } catch (e) { + if (e.code !== "ENOENT") throw e; + const code = reifyCompileWithCache(file); + process.nextTick(writeFileAtomically, cacheFileName, code); + return code; + } + } else { + return reifyCompileWithCache(file); + } + } + + json(file) { + file.jsonData = JSON.parse(file.dataString); return jsonDataToCommonJS(file.jsonData); - }, + } - ".css"(dataString, hash) { + css({ dataString, hash }) { return cssToCommonJS(dataString, hash); } -}; +} function jsonDataToCommonJS(data) { return "module.exports = " + @@ -184,6 +256,7 @@ export default class ImportScanner { sourceRoot, nodeModulesPaths = [], watchSet, + cacheDir, }) { assert.ok(isString(sourceRoot)); @@ -192,11 +265,17 @@ export default class ImportScanner { this.sourceRoot = sourceRoot; this.nodeModulesPaths = nodeModulesPaths; this.watchSet = watchSet; + this.cacheDir = cacheDir; this.absPathToOutputIndex = Object.create(null); this.realPathToFiles = Object.create(null); this.realPathCache = Object.create(null); this.allMissingModules = Object.create(null); this.outputFiles = []; + this.defaultHandlers = new DefaultHandlers({ + bundleArch, + sourceRoot, + cacheDir, + }); this.resolver = Resolver.getOrCreate({ caller: "ImportScanner#constructor", @@ -274,21 +353,22 @@ export default class ImportScanner { // system, but any import statements or require calls in file.data // will be interpreted relative to this path, so it needs to be // something plausible. #6411 #6383 - const absPath = pathJoin(this.sourceRoot, file.sourcePath); + file.absPath = pathJoin(this.sourceRoot, file.sourcePath); // This property can have values false, true, "dynamic" (which // indicates that the file has been imported, but only dynamically). file.imported = false; - file.absModuleId = file.absModuleId || this._getAbsModuleId(absPath); + file.absModuleId = file.absModuleId || + this._getAbsModuleId(file.absPath); - if (! this._addFile(absPath, file)) { + if (! this._addFile(file.absPath, file)) { // Collisions can happen if a compiler plugin calls addJavaScript // multiple times with the same sourcePath. #6422 - this._combineFiles(this._getFile(absPath), file); + this._combineFiles(this._getFile(file.absPath), file); } - this._addFileByRealPath(file, this._realPath(absPath)); + this._addFileByRealPath(file, this._realPath(file.absPath)); }); return this; @@ -936,16 +1016,18 @@ export default class ImportScanner { return file.dataString; } - const dotExt = "." + file.type; - const dataString = file.data.toString("utf8"); - file.dataString = defaultExtensionHandlers[dotExt].call( - file, - dataString, - file.hash, - ); + const rawDataString = file.data.toString("utf8"); + if (file.type === "js") { + // Avoid compiling .js file with Reify when all we want is a string + // version of file.data. + file.dataString = stripHashBang(rawDataString); + } else { + file.dataString = rawDataString; + file.dataString = this.defaultHandlers[file.type](file); + } if (! (file.data instanceof Buffer) || - file.dataString !== dataString) { + file.dataString !== rawDataString) { file.data = Buffer.from(file.dataString, "utf8"); } @@ -954,6 +1036,7 @@ export default class ImportScanner { _readFile(absPath) { const info = { + absPath, data: optimisticReadFile(absPath), hash: optimisticHashOrNull(absPath), }; @@ -1004,9 +1087,9 @@ export default class ImportScanner { } _readModule(absPath) { - let ext = pathExtname(absPath).toLowerCase(); + const dotExt = pathExtname(absPath).toLowerCase(); - if (ext === ".node") { + if (dotExt === ".node") { const dataString = "throw new Error(" + JSON.stringify( this.isWeb() ? "cannot load native .node modules on the client" @@ -1016,7 +1099,7 @@ export default class ImportScanner { const data = Buffer.from(dataString, "utf8"); const hash = sha1(data); - return { data, dataString, hash }; + return { absPath, data, dataString, hash }; } try { @@ -1028,20 +1111,16 @@ export default class ImportScanner { const dataString = info.dataString; - if (! has(defaultExtensionHandlers, ext)) { + let ext = dotExt.slice(1); + if (! has(DefaultHandlers.prototype, ext)) { if (canBeParsedAsPlainJS(dataString)) { - ext = ".js"; + ext = "js"; } else { return null; } } - info.dataString = defaultExtensionHandlers[ext].call( - info, - info.dataString, - info.hash, - ); - + info.dataString = this.defaultHandlers[ext](info); if (info.dataString !== dataString) { info.data = Buffer.from(info.dataString, "utf8"); }