From 5d0a1200c7321406ac53ad0bc7370c4b35041d81 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 3 May 2019 16:33:32 -0400 Subject: [PATCH 1/2] Allow .npm/package/node_modules to be compiled in Meteor packages. When I implemented support for the "module" entry point in package.json files for client code in #10541, I modified PackageSource#_findSources to include files found in node_modules that need to be compiled, but my implementation considered only "local" node_modules directories, like the one in the application root directory, while neglecting the private .npm/package/node_modules directories that many Meteor packages have. This commit includes .npm/**/node_modules when _findSources is scanning a Meteor package, which should solve issues like #10544, where a Meteor package imports an npm package that was installed with Npm.depends, and that npm package has a "module" field in its package.json file, pointing to an ESM entry point module, but the ESM syntax was not appropriately compiled, leading to parse errors like "Unexpected token export". Before lazy compilation was introduced in Meteor 1.7 (#9983), including the node_modules directories of Meteor packages would likely have been a big problem for build performance, since there would be that many more modules to compile. It's still worth making sure this change doesn't regress build performance for other reasons, but I'm reasonably confident lazy compilation will save us here, unless there are just too many npm packages installed via Npm.depends that export ESM modules. --- tools/isobuild/import-scanner.js | 21 ++++++++ tools/isobuild/package-source.js | 51 ++++++++++++++----- tools/isobuild/unibuild.js | 32 +++++++++++- .../.npm/package/npm-shrinkwrap.json | 5 ++ .../packages/modules-test-package/common.js | 10 ++++ .../packages/modules-test-package/package.js | 1 + .../plugin/compile-arson/npm-shrinkwrap.json | 40 +++++++++------ .../packages/modules-test-plugin/package.js | 1 + .../packages/modules-test-plugin/plugin.js | 28 ++++++---- 9 files changed, 150 insertions(+), 39 deletions(-) diff --git a/tools/isobuild/import-scanner.js b/tools/isobuild/import-scanner.js index 6e4b033449..b90a175fe4 100644 --- a/tools/isobuild/import-scanner.js +++ b/tools/isobuild/import-scanner.js @@ -375,6 +375,27 @@ 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 0cf365e7ed..9625d876a6 100644 --- a/tools/isobuild/package-source.js +++ b/tools/isobuild/package-source.js @@ -426,6 +426,9 @@ _.extend(PackageSource.prototype, { sourceRoot: self.sourceRoot, uses: _.map(options.use, splitConstraint), getFiles() { + // TODO We might want to call _findSources here, if we want plugins to + // be able to import compiled files that were not explicitly included in + // the sources array passed to Package.registerBuildPlugin. return { sources: sources } @@ -1298,10 +1301,6 @@ _.extend(PackageSource.prototype, { subdirectories.forEach(subdir => { if (/(^|\/)node_modules\/$/.test(subdir)) { - if (! inNodeModules) { - sourceArch.localNodeModulesDirs[subdir] = true; - } - // Defer handling node_modules until after we handle all other // subdirectories, so that we know whether we need to descend // further. If sources is still empty after we handle everything @@ -1309,21 +1308,29 @@ _.extend(PackageSource.prototype, { // imported by anthing outside of it, so we can ignore it. nodeModulesDir = subdir; + // A "local" node_modules directory is one that's managed by the + // application developer using npm, rather than by Meteor using + // Npm.depends, which is available only in Meteor packages, and + // installs its dependencies into .npm/*/node_modules. Local + // node_modules directories may contain other nested node_modules + // directories, but we care about recording only the top-level + // node_modules directories here (hence !inNodeModules). + if (!inNodeModules && (isApp || !subdir.startsWith(".npm/"))) { + sourceArch.localNodeModulesDirs[subdir] = true; + } + } else { sources.push(...find(subdir, depth + 1, inNodeModules)); } }); - if (isApp && - nodeModulesDir && - (! inNodeModules || sources.length > 0)) { + if (nodeModulesDir && (!inNodeModules || sources.length > 0)) { // If we found a node_modules subdirectory above, and either we // are not already inside another node_modules directory or we // found source files elsewhere in this directory or its other - // subdirectories, and we're building an app (as opposed to a - // Meteor package), continue searching this node_modules - // directory, so that any non-.js(on) files it contains can be - // imported by the app (#6037). + // subdirectories, continue searching this node_modules directory, + // so that any non-.js(on) files it contains can be imported by + // the app (#6037). sources.push(...find(nodeModulesDir, depth + 1, true)); } @@ -1336,7 +1343,27 @@ _.extend(PackageSource.prototype, { return sources; } - return files.withCache(() => find("", 0, false)); + 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; }), _findAssets({ diff --git a/tools/isobuild/unibuild.js b/tools/isobuild/unibuild.js index d3bc27a1a4..26c09e03e4 100644 --- a/tools/isobuild/unibuild.js +++ b/tools/isobuild/unibuild.js @@ -251,6 +251,7 @@ 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. @@ -258,6 +259,9 @@ 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); @@ -315,13 +319,37 @@ export class Unibuild { }); // Output other resources each to their own file - _.each(unibuild.resources, function (resource) { + _.each(unibuild.resources, resource => { if (_.contains(["head", "body"], resource.type)) { // already did this one return; } - const generatedFilename = + 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 || builder.writeToGeneratedFilename( files.pathJoin( unibuildDir, diff --git a/tools/tests/apps/modules/packages/modules-test-package/.npm/package/npm-shrinkwrap.json b/tools/tests/apps/modules/packages/modules-test-package/.npm/package/npm-shrinkwrap.json index 16f2dfd3ac..9d6b263e5e 100644 --- a/tools/tests/apps/modules/packages/modules-test-package/.npm/package/npm-shrinkwrap.json +++ b/tools/tests/apps/modules/packages/modules-test-package/.npm/package/npm-shrinkwrap.json @@ -1,6 +1,11 @@ { "lockfileVersion": 1, "dependencies": { + "@wry/context": { + "version": "0.4.0", + "resolved": "https://registry.npmjs.org/@wry/context/-/context-0.4.0.tgz", + "integrity": "sha512-rVjwzFjVYXJ8pWJ8ZRCHv6meOebQvfTlvnUYUNX93Ce0KNeMTqCkf0GiOJc6BNVB96s7qfvwoLN3nUgDnSFOOg==" + }, "assert": { "version": "1.3.0", "resolved": "https://registry.npmjs.org/assert/-/assert-1.3.0.tgz", diff --git a/tools/tests/apps/modules/packages/modules-test-package/common.js b/tools/tests/apps/modules/packages/modules-test-package/common.js index b9037f6771..b271372428 100644 --- a/tools/tests/apps/modules/packages/modules-test-package/common.js +++ b/tools/tests/apps/modules/packages/modules-test-package/common.js @@ -5,6 +5,16 @@ export const ModulesTestPackage = "loaded"; import { parse } from "acorn"; assert.strictEqual(typeof parse, "function"); +// Test that an npm package with a "module" entry point in its package.json +// file can be imported. +import { Slot } from "@wry/context"; +assert.strictEqual(typeof Slot, "function"); +const idPrefix = "/node_modules/meteor/modules-test-package/node_modules/@wry/context/lib/"; +assert.strictEqual( + require.resolve("@wry/context"), + idPrefix + (Meteor.isClient ? "context.esm.js" : "context.js"), +); + export function checkWhere(where) { const { where: serverWhere } = require("./server/where.js"); const { where: clientWhere } = require("./client/where.js"); diff --git a/tools/tests/apps/modules/packages/modules-test-package/package.js b/tools/tests/apps/modules/packages/modules-test-package/package.js index 022510cd73..2e44d7459c 100644 --- a/tools/tests/apps/modules/packages/modules-test-package/package.js +++ b/tools/tests/apps/modules/packages/modules-test-package/package.js @@ -6,6 +6,7 @@ Package.describe({ }); Npm.depends({ + "@wry/context": "0.4.0", "os-browserify": "0.2.0", "assert": "1.3.0", "cheerio": "0.22.0" diff --git a/tools/tests/apps/modules/packages/modules-test-plugin/.npm/plugin/compile-arson/npm-shrinkwrap.json b/tools/tests/apps/modules/packages/modules-test-plugin/.npm/plugin/compile-arson/npm-shrinkwrap.json index c019a5183d..ec85e9fa6b 100644 --- a/tools/tests/apps/modules/packages/modules-test-plugin/.npm/plugin/compile-arson/npm-shrinkwrap.json +++ b/tools/tests/apps/modules/packages/modules-test-plugin/.npm/plugin/compile-arson/npm-shrinkwrap.json @@ -17,19 +17,19 @@ "integrity": "sha1-wM5mIMtfLB+xArIPZXQRVcq8REo=" }, "babel-runtime": { - "version": "6.23.0", - "resolved": "https://registry.npmjs.org/babel-runtime/-/babel-runtime-6.23.0.tgz", - "integrity": "sha1-CpSJ8UTecO+zzkMArM2zKeL8VDs=" + "version": "6.26.0", + "resolved": "https://registry.npmjs.org/babel-runtime/-/babel-runtime-6.26.0.tgz", + "integrity": "sha1-llxwWGaOgrVde/4E/yM3vItWR/4=" }, "babel-types": { - "version": "6.25.0", - "resolved": "https://registry.npmjs.org/babel-types/-/babel-types-6.25.0.tgz", - "integrity": "sha1-cK+ySNVmDl0Y+BHZHIMDtUE0oY4=" + "version": "6.26.0", + "resolved": "https://registry.npmjs.org/babel-types/-/babel-types-6.26.0.tgz", + "integrity": "sha1-o7Bz+Uq0nrb6Vc1lInozQ4BjJJc=" }, "core-js": { - "version": "2.4.1", - "resolved": "https://registry.npmjs.org/core-js/-/core-js-2.4.1.tgz", - "integrity": "sha1-TekR5mew6ukSTjQlS1OupvxhjT4=" + "version": "2.6.5", + "resolved": "https://registry.npmjs.org/core-js/-/core-js-2.6.5.tgz", + "integrity": "sha512-klh/kDpwX8hryYL14M9w/xei6vrv6sE8gTHDG7/T/+SEovB/G4ejwcfE/CBzO6Edsu+OETZMZ3wcX/EjUkrl5A==" }, "esutils": { "version": "2.0.2", @@ -37,19 +37,29 @@ "integrity": "sha1-Cr9PHKpbyx96nYrMbepPqqBLrJs=" }, "lodash": { - "version": "4.17.4", - "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.4.tgz", - "integrity": "sha1-eCA6TRwyiuHYbcpkYONptX9AVa4=" + "version": "4.17.11", + "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.11.tgz", + "integrity": "sha512-cQKh8igo5QUhZ7lg38DYWAxMvjSAKG0A8wGSVimP07SIUEK2UO+arSRKbRZWtelMtN5V0Hkwh5ryOto/SshYIg==" }, "regenerator-runtime": { - "version": "0.10.5", - "resolved": "https://registry.npmjs.org/regenerator-runtime/-/regenerator-runtime-0.10.5.tgz", - "integrity": "sha1-M2w+/BIgrc7dosn6tntaeVWjNlg=" + "version": "0.11.1", + "resolved": "https://registry.npmjs.org/regenerator-runtime/-/regenerator-runtime-0.11.1.tgz", + "integrity": "sha512-MguG95oij0fC3QV3URf4V2SDYGJhJnJGqvIIgdECeODCT98wSWDAJ94SSuVpYQUoTcGUIL6L4yNB7j1DFFHSBg==" }, "to-fast-properties": { "version": "1.0.3", "resolved": "https://registry.npmjs.org/to-fast-properties/-/to-fast-properties-1.0.3.tgz", "integrity": "sha1-uDVx+k2MJbguIxsG46MFXeTKGkc=" + }, + "ts-invariant": { + "version": "0.4.1", + "resolved": "https://registry.npmjs.org/ts-invariant/-/ts-invariant-0.4.1.tgz", + "integrity": "sha512-fdL8AZinDiVKMsOI0cOWHLprS85LWy2p/eVSctVe6fpZF9BAvO59sQYMEWQ37yybBtlKU2zkmILYmy1jrOf6+g==" + }, + "tslib": { + "version": "1.9.3", + "resolved": "https://registry.npmjs.org/tslib/-/tslib-1.9.3.tgz", + "integrity": "sha512-4krF8scpejhaOgqzBEcGM7yDIEfi0/8+8zDRZhNZZ2kjmHJ4hv3zCbQWxoJGz1iw5U0Jl0nma13xzHXcncMavQ==" } } } diff --git a/tools/tests/apps/modules/packages/modules-test-plugin/package.js b/tools/tests/apps/modules/packages/modules-test-plugin/package.js index 3810cecf88..6a48b83086 100644 --- a/tools/tests/apps/modules/packages/modules-test-plugin/package.js +++ b/tools/tests/apps/modules/packages/modules-test-plugin/package.js @@ -14,6 +14,7 @@ Package.registerBuildPlugin({ npmDependencies: { // TODO Figure out what to do about the duplication between this list // and the Npm.depends list below. + "ts-invariant": "0.4.1", "babel-plugin-transform-class-properties": "6.9.0", "babel-plugin-transform-strict-mode": "6.8.0" } diff --git a/tools/tests/apps/modules/packages/modules-test-plugin/plugin.js b/tools/tests/apps/modules/packages/modules-test-plugin/plugin.js index e9f0d5313a..bfc6c8bfb3 100644 --- a/tools/tests/apps/modules/packages/modules-test-plugin/plugin.js +++ b/tools/tests/apps/modules/packages/modules-test-plugin/plugin.js @@ -1,4 +1,14 @@ -import assert from "assert"; +import { invariant } from "ts-invariant"; + +invariant( + typeof process.versions.node === "string", + "Meteor plugins should only run in Node.js", +); + +invariant( + require.resolve("ts-invariant"), + "/node_modules/meteor/meteor-test-plugin/node_modules/ts-invariant/lib/invariant.js", +); // This verifies that babel-plugin-transform-strict-mode is enabled. let expected; @@ -7,8 +17,8 @@ try { } catch (e) { expected = e; } -assert.ok(expected instanceof TypeError); -assert.ok(/callee/.test(expected.message)); +invariant(expected instanceof TypeError); +invariant(/callee/.test(expected.message), expected.message); Plugin.registerCompiler({ extensions: ["arson"] @@ -20,8 +30,8 @@ class ArsonCompiler { expectedName = "compile-arson"; processFilesForTarget(inputFiles) { - assert.strictEqual(this.expectedName, "compile-arson"); - assert.ok(inputFiles.length > 0); + invariant(this.expectedName === "compile-arson", this.expectedName); + invariant(inputFiles.length > 0); let vueCheckCount = 0; @@ -47,14 +57,12 @@ class ArsonCompiler { const vueCompilerId = file.resolve("vue-template-compiler"); // Make sure resolution does not use the "browser" field of // vue-template-compiler/package.json. - assert.strictEqual( - vueCompilerId.split("/").pop(), - "index.js" - ); + const base = vueCompilerId.split("/").pop(); + invariant(base === "index.js", base); ++vueCheckCount; } }); - assert.ok(vueCheckCount > 0); + invariant(vueCheckCount > 0); } } From 683d23cdee8ceea96219adb0ec0e41f2c8ade0bd Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 3 May 2019 18:01:50 -0400 Subject: [PATCH 2/2] Bump compiler.BUILT_BY and LINKER_CACHE_SALT to force rebuild. --- tools/isobuild/compiler-plugin.js | 2 +- tools/isobuild/compiler.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/isobuild/compiler-plugin.js b/tools/isobuild/compiler-plugin.js index 0ae055c57f..726cb34f4d 100644 --- a/tools/isobuild/compiler-plugin.js +++ b/tools/isobuild/compiler-plugin.js @@ -65,7 +65,7 @@ const hasOwn = Object.prototype.hasOwnProperty; // Cache the (slightly post-processed) results of linker.fullLink. const CACHE_SIZE = process.env.METEOR_LINKER_CACHE_SIZE || 1024*1024*100; const CACHE_DEBUG = !! process.env.METEOR_TEST_PRINT_LINKER_CACHE_DEBUG; -const LINKER_CACHE_SALT = 22; // Increment this number to force relinking. +const LINKER_CACHE_SALT = 23; // Increment this number to force relinking. const LINKER_CACHE = new LRU({ max: CACHE_SIZE, // Cache is measured in bytes. We don't care about servePath. diff --git a/tools/isobuild/compiler.js b/tools/isobuild/compiler.js index 4f8c5a2ca3..086310dc60 100644 --- a/tools/isobuild/compiler.js +++ b/tools/isobuild/compiler.js @@ -34,7 +34,7 @@ var compiler = exports; // dependencies. (At least for now, packages only used in target creation (eg // minifiers) don't require you to update BUILT_BY, though you will need to quit // and rerun "meteor run".) -compiler.BUILT_BY = 'meteor/32'; +compiler.BUILT_BY = 'meteor/33'; // This is a list of all possible architectures that a build can target. (Client // is expanded into 'web.browser' and 'web.cordova')