From a86c61eb9bd94fc70a4cb4bb5993cdcd0a9b89ba Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 29 Mar 2016 17:33:06 -0400 Subject: [PATCH] Don't warn about unresolved modules in Browserify/Webpack bundle files. Fixes https://github.com/practicalmeteor/meteor-mocha/issues/19 Related: https://github.com/meteor/todos/issues/90 See also: 62c41b2ef579cc3a301aabb660eeae77cb227e4c --- tools/isobuild/compiler-plugin.js | 27 ++++++-- tools/isobuild/import-scanner.js | 108 +++++++++++++++++------------- tools/isobuild/js-analyze.js | 50 +++++++++++--- tools/isobuild/linker.js | 8 ++- 4 files changed, 131 insertions(+), 62 deletions(-) diff --git a/tools/isobuild/compiler-plugin.js b/tools/isobuild/compiler-plugin.js index 07042bd457..29e0fc1177 100644 --- a/tools/isobuild/compiler-plugin.js +++ b/tools/isobuild/compiler-plugin.js @@ -57,7 +57,7 @@ import { isTestFilePath } from './test-files.js'; // 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 = 2; // Increment this number to force relinking. +const LINKER_CACHE_SALT = 3; // 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. @@ -766,7 +766,7 @@ export class PackageSourceBatch { function handleMissing(missingNodeModules) { const missingMap = new Map; - Object.keys(missingNodeModules).forEach(id => { + _.each(missingNodeModules, (info, id) => { const parts = id.split("/"); let name = null; @@ -784,10 +784,16 @@ export class PackageSourceBatch { return; } - if (missingMap.has(name)) { - missingMap.get(name).push(id); - } else { - missingMap.set(name, [id]); + if (! missingMap.has(name)) { + missingMap.set(name, {}); + } + + const missing = missingMap.get(name); + if (! _.has(missing, id) || + ! info.possiblySpurious) { + // Allow any non-spurious identifier to replace an existing + // possibly spurious identifier. + missing[id] = info; } }); @@ -807,7 +813,7 @@ export class PackageSourceBatch { handleMissing(allMissingNodeModules); - _.each(allRelocatedNodeModules, (packageName, id) => { + _.each(allRelocatedNodeModules, (info, id) => { delete allMissingNodeModules[id]; }); @@ -855,6 +861,13 @@ export class PackageSourceBatch { const warnings = []; _.each(missingNodeModules, (info, id) => { + if (info.possiblySpurious) { + // Silence warnings for missing dependencies in Browserify/Webpack + // bundles, since we can reasonably conclude at this point that + // they are false positives. + return; + } + if (info.packageName === "avital:mocha") { // Silence warnings for the avital:mocha package, because we know // they're spurious, and it seems poor form to spew a bunch of diff --git a/tools/isobuild/import-scanner.js b/tools/isobuild/import-scanner.js index 8dd95b0550..52c804024f 100644 --- a/tools/isobuild/import-scanner.js +++ b/tools/isobuild/import-scanner.js @@ -1,5 +1,7 @@ import assert from "assert"; -import {isString, has, keys, each, map, without} from "underscore"; +import { + isString, isEmpty, 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"; @@ -65,9 +67,9 @@ const defaultExtensionHandlers = { // of ImportScanner (which do not persist across builds). const IMPORT_SCANNER_CACHE = new LRU({ max: 1024*1024, - length(value) { + length(ids) { let total = 40; // size of key - value.forEach(str => { total += str.length; }); + each(ids, (info, id) => { total += id.length; }); return total; } }); @@ -199,49 +201,46 @@ export default class ImportScanner { } addNodeModules(identifiers) { + assert.ok(identifiers); + assert.ok(typeof identifiers === "object"); + assert.ok(! Array.isArray(identifiers)); + const newlyMissing = Object.create(null); const newlyAdded = Object.create(null); - if (identifiers) { - if (typeof identifiers === "object" && - ! Array.isArray(identifiers)) { - identifiers = Object.keys(identifiers); - } + if (! isEmpty(identifiers)) { + const previousAllMissingNodeModules = this.allMissingNodeModules; + this.allMissingNodeModules = newlyMissing; - if (identifiers.length > 0) { - const previousAllMissingNodeModules = this.allMissingNodeModules; - this.allMissingNodeModules = newlyMissing; + try { + this._scanFile({ + sourcePath: "fake.js", + // By specifying the .deps property of this fake file ahead of + // time, we can avoid calling findImportedModuleIdentifiers in the + // _scanFile method. + deps: identifiers, + }); - try { - this._scanFile({ - sourcePath: "fake.js", - // By specifying the .deps property of this fake file ahead of - // time, we can avoid calling findImportedModuleIdentifiers in the - // _scanFile method. - deps: identifiers, - }); + } finally { + this.allMissingNodeModules = previousAllMissingNodeModules; - } finally { - this.allMissingNodeModules = previousAllMissingNodeModules; + each(identifiers, (info, id) => { + if (! has(newlyMissing, id)) { + newlyAdded[id] = info; + } + }); - identifiers.forEach(id => { - if (! has(newlyMissing, id)) { - newlyAdded[id] = this.name; - } - }); - - // Remove previously seen missing module identifiers from - // newlyMissing and merge the new identifiers back into - // this.allMissingNodeModules. - each(keys(newlyMissing), key => { - if (has(previousAllMissingNodeModules, key)) { - delete newlyMissing[key]; - } else { - previousAllMissingNodeModules[key] = - newlyMissing[key]; - } - }); - } + // Remove previously seen missing module identifiers from + // newlyMissing and merge the new identifiers back into + // this.allMissingNodeModules. + each(keys(newlyMissing), key => { + if (has(previousAllMissingNodeModules, key)) { + delete newlyMissing[key]; + } else { + previousAllMissingNodeModules[key] = + newlyMissing[key]; + } + }); } } @@ -302,10 +301,10 @@ export default class ImportScanner { return IMPORT_SCANNER_CACHE.get(file.hash); } - const result = keys(findImportedModuleIdentifiers( + const result = findImportedModuleIdentifiers( file.dataString, file.hash, - )); + ); // there should always be file.hash, but better safe than sorry if (file.hash) { @@ -332,7 +331,7 @@ export default class ImportScanner { throw e; } - each(file.deps, id => { + each(file.deps, (info, id) => { const resolved = this._tryToResolveImportedPath(file, id); if (! resolved) { return; @@ -702,8 +701,12 @@ export default class ImportScanner { const isApp = ! this.name; if (isApp && isNative && archMatches(this.bundleArch, "web")) { // To ensure the native module can be evaluated at runtime, - // register a dependency on meteor-node-stubs/defs/.js. - return this._resolveNodeModule(file, nativeModulesMap[id]); + // register a dependency on meteor-node-stubs/deps/.js. + const stubsId = nativeModulesMap[id]; + if (isString(stubsId) && stubsId !== id) { + file.deps[stubsId] = file.deps[id]; + return this._resolveNodeModule(file, stubsId); + } } // If the imported identifier is neither absolute nor relative, but @@ -712,11 +715,24 @@ export default class ImportScanner { // missing dependency so that we can include it in the app bundle. const missing = file.missingNodeModules || Object.create(null); file.missingNodeModules = missing; - this.allMissingNodeModules[id] = missing[id] = { + + const possiblySpurious = + has(file.deps, id) && + file.deps[id].possiblySpurious; + + const info = missing[id] = { packageName: this.name, parentPath: absPath, bundleArch: this.bundleArch, + possiblySpurious, }; + + if (! has(this.allMissingNodeModules, id) || + ! info.possiblySpurious) { + // Allow any non-spurious identifier to replace an existing + // possibly spurious identifier. + this.allMissingNodeModules[id] = info; + } } // If the dependency is still not resolved, it might be handled by the @@ -805,7 +821,7 @@ export default class ImportScanner { const pkgFile = { type: "js", // We represent the JSON module with JS. data, - deps: [], // Avoid accidentally re-scanning this file. + deps: {}, // Avoid accidentally re-scanning this file. sourcePath: relPkgJsonPath, installPath: this._getInstallPath(pkgJsonPath), servePath: relPkgJsonPath, diff --git a/tools/isobuild/js-analyze.js b/tools/isobuild/js-analyze.js index 97b0c8f0ff..426db13b52 100644 --- a/tools/isobuild/js-analyze.js +++ b/tools/isobuild/js-analyze.js @@ -2,6 +2,8 @@ import { parse } from 'meteor-babel'; import { analyze as analyzeScope } from 'escope'; import LRU from "lru-cache"; +const hasOwn = Object.prototype.hasOwnProperty; + var AST_CACHE = new LRU({ max: Math.pow(2, 12), length(ast) { @@ -65,13 +67,13 @@ export function findImportedModuleIdentifiers(source, hash) { const ast = tryToParse(source, hash); - function walk(node, left, right) { + function walk(node, left, right, requireIsBound) { if (left >= right) { // The window of possible indexes is empty, so we can ignore // the entire subtree rooted at this node. } else if (Array.isArray(node)) { for (var i = 0, len = node.length; i < len; ++i) { - walk(node[i], left, right); + walk(node[i], left, right, requireIsBound); } } else if (isNode(node)) { const start = node.start; @@ -83,16 +85,19 @@ export function findImportedModuleIdentifiers(source, hash) { while (left < right && end < possibleIndexes[right - 1]) --right; if (left < right) { + if (! requireIsBound && + isFunctionWithParameter(node, "require")) { + requireIsBound = true; + } + let id = getRequiredModuleId(node); if (typeof id === "string") { - identifiers[id] = node; - return; + return addIdentifier(id, "require", requireIsBound); } id = getImportedModuleId(node); if (typeof id === "string") { - identifiers[id] = node; - return; + return addIdentifier(id, "import", requireIsBound); } // Continue traversing the children of this node. @@ -106,13 +111,30 @@ export function findImportedModuleIdentifiers(source, hash) { continue; } - walk(node[key], left, right); + walk(node[key], left, right, requireIsBound); } } } } - walk(ast, 0, possibleIndexes.length); + function addIdentifier(id, type, requireIsBound) { + const entry = hasOwn.call(identifiers, id) + ? identifiers[id] + : identifiers[id] = { possiblySpurious: true }; + + if (type === "require") { + // If the identifier comes from a require call, but require is not a + // free variable, then this dependency might be spurious. + entry.possiblySpurious = + entry.possiblySpurious && requireIsBound; + } else { + // The import keyword can't be shadowed, so any dependencies + // registered by import statements should be trusted absolutely. + entry.possiblySpurious = false; + } + } + + walk(ast, 0, possibleIndexes.length, false); return identifiers; } @@ -125,6 +147,18 @@ function isNode(value) { && typeof value.end === "number"; } +function isFunctionWithParameter(node, name) { + if (node.type === "FunctionExpression" || + node.type === "FunctionDeclaration" || + node.type === "ArrowFunctionExpression") { + return node.params.some( + param => + param.type === "Identifier" && + param.name === name + ); + } +} + function getRequiredModuleId(node) { if (node.type === "CallExpression" && node.callee.type === "Identifier" && diff --git a/tools/isobuild/linker.js b/tools/isobuild/linker.js index 8a3ca04d12..008a5346f1 100644 --- a/tools/isobuild/linker.js +++ b/tools/isobuild/linker.js @@ -423,7 +423,13 @@ var File = function (inputFile, module) { self.servePath = inputFile.servePath; // Module identifiers imported or required by this module, if any. - self.deps = inputFile.deps || []; + if (Array.isArray(inputFile.deps)) { + self.deps = inputFile.deps; + } else if (inputFile.deps && typeof inputFile.deps === "object") { + self.deps = Object.keys(inputFile.deps); + } else { + self.deps = []; + } // True if the input file should not be evaluated eagerly. self.lazy = inputFile.lazy; // could be `true`, `false` or `undefined`