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/isobuild/compiler-plugin.js b/tools/isobuild/compiler-plugin.js index 415a1facf1..4646f3f76f 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; } @@ -634,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); }); @@ -662,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; @@ -692,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 => { @@ -723,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 127ed086fb..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"; @@ -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") { @@ -460,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 @@ -502,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; } } @@ -528,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); 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]); 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/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/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 f45dfabd23..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,8 +5,13 @@ Package.describe({ documentation: 'README.md' }); +Npm.depends({ + "os-browserify": "0.2.0" +}); + Package.onUse(function(api) { api.use('ecmascript'); + api.use('templating'); api.mainModule("client.js", "client"); api.mainModule("server.js", "server"); api.export("ModulesTestPackage"); diff --git a/tools/tests/apps/modules/tests.js b/tools/tests/apps/modules/tests.js index 91447b64a6..9cfa395bec 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; @@ -210,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", () => {