From 143121d644d866f0e5f03dc2c731ac2c43cef6dc Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 9 Feb 2016 13:39:22 -0500 Subject: [PATCH 1/7] Call scanner.addInputFiles even if the app is not using modules. --- tools/isobuild/compiler-plugin.js | 6 +++--- tools/isobuild/import-scanner.js | 4 ++++ 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/tools/isobuild/compiler-plugin.js b/tools/isobuild/compiler-plugin.js index 95a7659b6e..415a1facf1 100644 --- a/tools/isobuild/compiler-plugin.js +++ b/tools/isobuild/compiler-plugin.js @@ -682,10 +682,10 @@ export class PackageSourceBatch { watchSet: batch.unibuild.watchSet, }); - if (batch.useMeteorInstall) { - scanner.addInputFiles(map.get(name)); + scanner.addInputFiles(map.get(name)); - scanner.getOutputFiles().forEach(file => { + if (batch.useMeteorInstall) { + scanner.scanImports().getOutputFiles().forEach(file => { if (file.missingNodeModules) { _.extend(allMissingNodeModules, file.missingNodeModules); } diff --git a/tools/isobuild/import-scanner.js b/tools/isobuild/import-scanner.js index 0ee4683f15..127ed086fb 100644 --- a/tools/isobuild/import-scanner.js +++ b/tools/isobuild/import-scanner.js @@ -90,6 +90,10 @@ export default class ImportScanner { } }); + return this; + } + + scanImports() { this.outputFiles.forEach(file => { if (! file.lazy || file.imported) { this._scanFile(file); From bd144c4cc6d709091f1ea7793a6da9437a6317e2 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 10 Feb 2016 16:43:15 -0500 Subject: [PATCH 2/7] Properly exclude lazy files when prelinking. Returning undefined from the _.map callback was leading to problems in code that iterated over the resulting list. --- tools/isobuild/linker.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tools/isobuild/linker.js b/tools/isobuild/linker.js index 7928f1f2dd..dd549007b3 100644 --- a/tools/isobuild/linker.js +++ b/tools/isobuild/linker.js @@ -112,12 +112,10 @@ _.extend(Module.prototype, { // preserving the line numbers. if (self.useGlobalNamespace && ! self.useMeteorInstall) { - return _.map(self.files, function (file) { - if (file.lazy) { - // Ignore lazy files unless we have a module system. - return; - } + // Ignore lazy files unless we have a module system. + const eagerFiles = _.filter(self.files, file => ! file.lazy); + return _.map(eagerFiles, function (file) { const cacheKey = JSON.stringify([ file.sourceHash, file.bare, file.servePath]); From 2a3fcf2e7bd1f43e2228684e07e8c1917807d6ef Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 10 Feb 2016 17:07:12 -0500 Subject: [PATCH 3/7] Remove optimization that classified all .json files as lazy. Fixes #6014. Fixes #6174. --- tools/isobuild/compiler-plugin.js | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/tools/isobuild/compiler-plugin.js b/tools/isobuild/compiler-plugin.js index 415a1facf1..d39e783d87 100644 --- a/tools/isobuild/compiler-plugin.js +++ b/tools/isobuild/compiler-plugin.js @@ -379,20 +379,12 @@ class ResourceSlot { return lazy; } - const sourcePath = this.inputResource.path; - - if (sourcePath.endsWith(".json")) { - // JSON files have no side effects, so there is no reason for them - // ever to be evaluated eagerly. - return true; - } - // If file.lazy was not previously defined, mark the file lazy if it // is contained by an imports directory. Note that any files contained // by a node_modules directory will already have been marked lazy in // PackageSource#_inferFileOptions. return this.packageSourceBatch.useMeteorInstall && - files.pathDirname(sourcePath) + files.pathDirname(this.inputResource.path) .split(files.pathSep) .indexOf("imports") >= 0; } From 1437e07ee55856fdaa66667120b6791964d745de Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 10 Feb 2016 18:26:52 -0500 Subject: [PATCH 4/7] Forbid server bundles from importing client-only files. This partially reverts 35a02864aff85c829cb509f2bcbb8dfe67a64848, and also fixes #6182. --- tools/isobuild/import-scanner.js | 33 ++++++++++++++++-------- tools/isobuild/package-source.js | 1 + tools/tests/apps/modules/client/eager.js | 1 - tools/tests/apps/modules/client/only.js | 1 + tools/tests/apps/modules/tests.js | 29 ++++++++++++++++----- 5 files changed, 47 insertions(+), 18 deletions(-) delete mode 100644 tools/tests/apps/modules/client/eager.js create mode 100644 tools/tests/apps/modules/client/only.js diff --git a/tools/isobuild/import-scanner.js b/tools/isobuild/import-scanner.js index 127ed086fb..24ec02b92c 100644 --- a/tools/isobuild/import-scanner.js +++ b/tools/isobuild/import-scanner.js @@ -125,8 +125,11 @@ export default class ImportScanner { } getOutputFiles(options) { - // Return all installable output files. - return this.outputFiles.filter(file => !! file.installPath); + // Return all installable output files that are either eager or + // imported by another module. + return this.outputFiles.filter(file => { + return file.installPath && (! file.lazy || file.imported); + }); } _findImportedModuleIdentifiers(file) { @@ -306,9 +309,8 @@ export default class ImportScanner { } const dirs = this._splitPath(pathDirname(installPath)); - const bundlingClientApp = - ! this.name && // Indicates we are bundling an app. - archMatches(this.bundleArch, "web"); + const isApp = ! this.name; + const bundlingForWeb = archMatches(this.bundleArch, "web"); for (let dir of dirs) { if (dir.charAt(0) === "." || @@ -319,12 +321,21 @@ export default class ImportScanner { return; } - if (bundlingClientApp && (dir === "server" || - dir === "private")) { - // If we're bundling an app for a client architecture, any files - // contained by a server-only directory that is not contained by - // a node_modules directory must be ignored. - return; + if (isApp) { + if (bundlingForWeb) { + if (dir === "server" || + dir === "private") { + // If we're bundling an app for a client architecture, any files + // contained by a server-only directory that is not contained by + // a node_modules directory must be ignored. + return; + } + } else if (dir === "client") { + // If we're bundling an app for a server architecture, any files + // contained by a client-only directory that is not contained by + // a node_modules directory must be ignored. + return; + } } if (dir === "node_modules") { diff --git a/tools/isobuild/package-source.js b/tools/isobuild/package-source.js index 2652a16353..7665e4beda 100644 --- a/tools/isobuild/package-source.js +++ b/tools/isobuild/package-source.js @@ -1383,6 +1383,7 @@ _.extend(PackageSource.prototype, { const anyLevelExcludes = [ /^tests\/$/, /^node_modules\/$/, + arch === "os" ? /^client\/$/ : /^server\/$/, ...sourceReadOptions.exclude ]; diff --git a/tools/tests/apps/modules/client/eager.js b/tools/tests/apps/modules/client/eager.js deleted file mode 100644 index f3e9aa9c6f..0000000000 --- a/tools/tests/apps/modules/client/eager.js +++ /dev/null @@ -1 +0,0 @@ -export const name = module.id; diff --git a/tools/tests/apps/modules/client/only.js b/tools/tests/apps/modules/client/only.js new file mode 100644 index 0000000000..3e3d4356d3 --- /dev/null +++ b/tools/tests/apps/modules/client/only.js @@ -0,0 +1 @@ +export default module.id; diff --git a/tools/tests/apps/modules/tests.js b/tools/tests/apps/modules/tests.js index 91447b64a6..89bd4fc373 100644 --- a/tools/tests/apps/modules/tests.js +++ b/tools/tests/apps/modules/tests.js @@ -5,12 +5,6 @@ import {Meteor as ImportedMeteor} from "meteor/meteor"; describe("app modules", () => { it("can be imported using absolute identifiers", () => { assert.strictEqual(require("/tests"), exports); - assert.strictEqual( - // Client modules should be importable by server modules, though not - // vice-versa. - require("/client/eager").name, - "/client/eager.js" - ); }); it("can have different file extensions", () => { @@ -62,6 +56,8 @@ describe("app modules", () => { if (Meteor.isServer) { assert.strictEqual(typeof error, "undefined"); assert.strictEqual(result, "/server/only.js"); + assert.strictEqual(require("./server/only"), + require("/server/only")); } if (Meteor.isClient) { @@ -69,6 +65,27 @@ describe("app modules", () => { } }); + it("cannot import client modules on server", () => { + let error; + let result; + try { + result = require("./client/only").default; + } catch (expectedOnServer) { + error = expectedOnServer; + } + + if (Meteor.isClient) { + assert.strictEqual(typeof error, "undefined"); + assert.strictEqual(result, "/client/only.js"); + assert.strictEqual(require("./client/only"), + require("/client/only")); + } + + if (Meteor.isServer) { + assert.ok(error instanceof Error); + } + }); + it("should not be parsed in strictMode", () => { let foo = 1234; delete foo; From ce4fda3783373d3a477496fe89655c64275118ae Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Thu, 11 Feb 2016 11:29:54 -0500 Subject: [PATCH 5/7] Tolerate compilation errors for lazy files. Fixes #5998. --- tools/isobuild/build-plugin.js | 11 +++++++++++ .../packages/modules-test-package/illegal.html | 1 + .../modules/packages/modules-test-package/package.js | 1 + 3 files changed, 13 insertions(+) create mode 100644 tools/tests/apps/modules/packages/modules-test-package/illegal.html diff --git a/tools/isobuild/build-plugin.js b/tools/isobuild/build-plugin.js index daf1ef06db..e8456e8e80 100644 --- a/tools/isobuild/build-plugin.js +++ b/tools/isobuild/build-plugin.js @@ -473,6 +473,17 @@ _.extend(exports.InputFile.prototype, { */ error: function (options) { var self = this; + if (self.getFileOptions().lazy === true) { + // Files with fileOptions.lazy === true were not explicitly added to + // the source batch via api.addFiles or api.mainModule, so any + // compilation errors should not be fatal. Attempting compilation is + // still important for lazy files that might end up being imported + // later, which is why we defang the error here, instead of avoiding + // compilation preemptively. Note also that actual exceptions will + // still cause build errors. + return; + } + var path = self.getPathInPackage(); var packageName = self.getPackageName(); if (packageName) { diff --git a/tools/tests/apps/modules/packages/modules-test-package/illegal.html b/tools/tests/apps/modules/packages/modules-test-package/illegal.html new file mode 100644 index 0000000000..e7994d980c --- /dev/null +++ b/tools/tests/apps/modules/packages/modules-test-package/illegal.html @@ -0,0 +1 @@ +
This is not a valid template file!
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 f45dfabd23..65697d5fe8 100644 --- a/tools/tests/apps/modules/packages/modules-test-package/package.js +++ b/tools/tests/apps/modules/packages/modules-test-package/package.js @@ -7,6 +7,7 @@ Package.describe({ Package.onUse(function(api) { api.use('ecmascript'); + api.use('templating'); api.mainModule("client.js", "client"); api.mainModule("server.js", "server"); api.export("ModulesTestPackage"); From 5d0a8f8f0e52a1fb73f8c900e6b9cf5aadc76370 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 9 Feb 2016 21:02:57 -0500 Subject: [PATCH 6/7] Allow apps and packages to import "meteor//..." modules. --- tools/isobuild/compiler-plugin.js | 58 +++++++++++++------ tools/isobuild/import-scanner.js | 17 +++--- .../packages/modules-test-package/os-stub.js | 2 + .../packages/modules-test-package/package.js | 4 ++ tools/tests/apps/modules/tests.js | 14 +++++ 5 files changed, 67 insertions(+), 28 deletions(-) create mode 100644 tools/tests/apps/modules/packages/modules-test-package/os-stub.js diff --git a/tools/isobuild/compiler-plugin.js b/tools/isobuild/compiler-plugin.js index d39e783d87..4646f3f76f 100644 --- a/tools/isobuild/compiler-plugin.js +++ b/tools/isobuild/compiler-plugin.js @@ -626,19 +626,11 @@ export class PackageSourceBatch { // Returns a map from package names to arrays of JS output files. static computeJsOutputFilesMap(sourceBatches) { const map = new Map; - const pkgSourceBatches = []; - let appSourceBatch; sourceBatches.forEach(batch => { const name = batch.unibuild.pkg.name || null; const inputFiles = []; - if (name) { - pkgSourceBatches.push(batch); - } else { - appSourceBatch = batch; - } - batch.resourceSlots.forEach(slot => { inputFiles.push(...slot.jsOutputResources); }); @@ -654,8 +646,9 @@ export class PackageSourceBatch { } const allMissingNodeModules = Object.create(null); + const scannerMap = new Map; - function scan(batch) { + sourceBatches.forEach(batch => { const name = batch.unibuild.pkg.name || null; const isApp = ! name; @@ -684,9 +677,44 @@ export class PackageSourceBatch { }); } - if (isApp) { - scanner.addNodeModules(allMissingNodeModules); + scannerMap.set(name, scanner); + }); + const missingMap = new Map; + + Object.keys(allMissingNodeModules).forEach(id => { + const parts = id.split("/"); + let name = null; + + if (parts[0] === "meteor") { + if (parts.length > 2) { + name = parts[1]; + parts[1] = "."; + id = parts.slice(1).join("/"); + } else { + return; + } + } + + if (! scannerMap.has(name)) { + return; + } + + if (missingMap.has(name)) { + missingMap.get(name).push(id); + } else { + missingMap.set(name, [id]); + } + }); + + missingMap.forEach((ids, name) => { + scannerMap.get(name).addNodeModules(ids); + }); + + scannerMap.forEach((scanner, name) => { + const isApp = ! name; + + if (isApp) { const appFilesWithoutNodeModules = []; scanner.getOutputFiles().forEach(file => { @@ -715,13 +743,7 @@ export class PackageSourceBatch { } else { map.set(name, scanner.getOutputFiles()); } - } - - pkgSourceBatches.forEach(scan); - - if (appSourceBatch) { - scan(appSourceBatch); - } + }); return map; } diff --git a/tools/isobuild/import-scanner.js b/tools/isobuild/import-scanner.js index 24ec02b92c..1ebcc4e064 100644 --- a/tools/isobuild/import-scanner.js +++ b/tools/isobuild/import-scanner.js @@ -471,16 +471,13 @@ export default class ImportScanner { if (! resolved) { const parts = id.split("/"); - if (parts[0] !== "meteor") { // Exclude meteor/... packages. - // If the imported identifier is neither absolute nor relative, - // but top-level, then it might be satisfied by a package - // installed in the top-level node_modules directory, and we - // should record the missing dependency so that we can include it - // in the app bundle. - const missing = file.missingNodeModules || Object.create(null); - missing[id] = true; - file.missingNodeModules = missing; - } + // If the imported identifier is neither absolute nor relative, but + // top-level, then it might be satisfied by a package installed in + // the top-level node_modules directory, and we should record the + // missing dependency so that we can include it in the app bundle. + const missing = file.missingNodeModules || Object.create(null); + missing[id] = true; + file.missingNodeModules = missing; } // If the dependency is still not resolved, it might be handled by the diff --git a/tools/tests/apps/modules/packages/modules-test-package/os-stub.js b/tools/tests/apps/modules/packages/modules-test-package/os-stub.js new file mode 100644 index 0000000000..b809de7c4c --- /dev/null +++ b/tools/tests/apps/modules/packages/modules-test-package/os-stub.js @@ -0,0 +1,2 @@ +export {platform} from "os-browserify/browser"; +export const name = module.id; 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 65697d5fe8..aba5380688 100644 --- a/tools/tests/apps/modules/packages/modules-test-package/package.js +++ b/tools/tests/apps/modules/packages/modules-test-package/package.js @@ -5,6 +5,10 @@ Package.describe({ documentation: 'README.md' }); +Npm.depends({ + "os-browserify": "0.2.0" +}); + Package.onUse(function(api) { api.use('ecmascript'); api.use('templating'); diff --git a/tools/tests/apps/modules/tests.js b/tools/tests/apps/modules/tests.js index 89bd4fc373..9cfa395bec 100644 --- a/tools/tests/apps/modules/tests.js +++ b/tools/tests/apps/modules/tests.js @@ -227,6 +227,20 @@ describe("Meteor packages", () => { const mtp = require("meteor/modules-test-package"); assert.strictEqual(mtp.where, Meteor.isServer ? "server" : "client"); }); + + it("should expose their files for import", () => { + const osStub = require("meteor/modules-test-package/os-stub"); + + assert.strictEqual( + osStub.platform(), + "browser" + ); + + assert.strictEqual( + osStub.name, + "/node_modules/meteor/modules-test-package/os-stub.js" + ); + }); }); describe("async functions", () => { From 3897fa8634f5f33c6de87cd26a038c76bf5813b8 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Wed, 10 Feb 2016 14:02:04 -0500 Subject: [PATCH 7/7] Respect string-valued "browser" field of package.json files. Fixes #6207. --- tools/isobuild/import-scanner.js | 43 +++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/tools/isobuild/import-scanner.js b/tools/isobuild/import-scanner.js index 1ebcc4e064..8108e62462 100644 --- a/tools/isobuild/import-scanner.js +++ b/tools/isobuild/import-scanner.js @@ -1,5 +1,5 @@ import assert from "assert"; -import {isString, has, keys, each, without} from "underscore"; +import {isString, has, keys, each, map, without} from "underscore"; import {sha1, readAndWatchFileWithHash} from "../fs/watch.js"; import {matches as archMatches} from "../utils/archinfo.js"; import {findImportedModuleIdentifiers} from "./js-analyze.js"; @@ -510,23 +510,46 @@ export default class ImportScanner { _resolvePkgJsonMain(dirPath, seenDirPaths) { const pkgJsonPath = pathJoin(dirPath, "package.json"); const pkg = this._readPkgJson(pkgJsonPath); + if (! pkg) { + return null; + } - if (pkg && isString(pkg.main)) { + let main = pkg.main; + + if (archMatches(this.bundleArch, "web") && + isString(pkg.browser)) { + main = pkg.browser; + } + + if (isString(main)) { // The "main" field of package.json does not have to begin with ./ // to be considered relative, so first we try simply appending it to // the directory path before falling back to a full resolve, which // might return a package from a node_modules directory. - const resolved = this._joinAndStat(dirPath, pkg.main) || + const resolved = this._joinAndStat(dirPath, main) || // The _tryToResolveImportedPath method takes a file object as its // first parameter, but only the .sourcePath property is ever // used, so we can get away with passing a fake directory file // object with only that property. this._tryToResolveImportedPath({ sourcePath: dirPath, - }, pkg.main, seenDirPaths); + }, main, seenDirPaths); if (resolved) { - this._addPkgJsonToOutput(pkgJsonPath, pkg); + // Output a JS module that exports just the "name", "version", and + // "main" properties defined in the package.json file. + const pkgSubset = { + name: pkg.name, + }; + + if (has(pkg, "version")) { + pkgSubset.version = pkg.version; + } + + pkgSubset.main = main; + + this._addPkgJsonToOutput(pkgJsonPath, pkgSubset); + return resolved; } } @@ -536,13 +559,9 @@ export default class ImportScanner { _addPkgJsonToOutput(pkgJsonPath, pkg) { if (! has(this.absPathToOutputIndex, pkgJsonPath)) { - const data = new Buffer( - // Output a JS module that exports just the "name", "version", and - // "main" properties defined in the package.json file. - "exports.name = " + JSON.stringify(pkg.name) + ";\n" + - "exports.version = " + JSON.stringify(pkg.version) + ";\n" + - "exports.main = " + JSON.stringify(pkg.main) + ";\n" - ); + const data = new Buffer(map(pkg, (value, key) => { + return `exports.${key} = ${JSON.stringify(value)};\n`; + }).join("")); const relPkgJsonPath = pathRelative(this.sourceRoot, pkgJsonPath);