From 8a57f6d2f47d2f4e6e119bb980bb965a8c232588 Mon Sep 17 00:00:00 2001 From: aleclarson Date: Fri, 1 Feb 2019 17:07:58 -0500 Subject: [PATCH] feat: package deduplication Packages with the same name/version pair are deduplicated. Closes #350 --- packages/metro-resolver/src/resolve.js | 151 +++++------------- packages/metro-resolver/src/types.js | 8 +- .../metro/src/node-haste/DependencyGraph.js | 2 +- .../DependencyGraph/ModuleResolution.js | 35 ++-- .../DependencyGraph/ResolutionRequest.js | 1 + packages/metro/src/node-haste/ModuleCache.js | 35 +++- packages/metro/src/node-haste/Package.js | 26 +-- 7 files changed, 108 insertions(+), 150 deletions(-) diff --git a/packages/metro-resolver/src/resolve.js b/packages/metro-resolver/src/resolve.js index e82ab15520..f3439594b3 100644 --- a/packages/metro-resolver/src/resolve.js +++ b/packages/metro-resolver/src/resolve.js @@ -14,7 +14,6 @@ const FailedToResolveNameError = require('./FailedToResolveNameError'); const FailedToResolvePathError = require('./FailedToResolvePathError'); const InvalidPackageError = require('./InvalidPackageError'); -const formatFileCandidates = require('./formatFileCandidates'); const isAbsolutePath = require('absolute-path'); const makePnpResolver = require('./makePnpResolver'); const path = require('path'); @@ -27,7 +26,6 @@ import type { FileContext, FileOrDirContext, FileResolution, - HasteContext, ModulePathContext, ResolutionContext, Resolution, @@ -35,6 +33,12 @@ import type { Result, } from './types'; +type ModuleParts = { + +package: string, + +scope: string, + +file: string, +}; + function resolve( context: ResolutionContext, moduleName: string, @@ -62,21 +66,21 @@ function resolve( return {type: 'empty'}; } - const {originModulePath} = context; - + const normalizedName = normalizePath(realModuleName); const isDirectImport = - isRelativeImport(realModuleName) || isAbsolutePath(realModuleName); + isRelativeImport(normalizedName) || isAbsolutePath(normalizedName); // We disable the direct file loading to let the custom resolvers deal with it if (!resolveRequest && isDirectImport) { - // derive absolute path /.../node_modules/originModuleDir/realModuleName + // derive absolute path /.../node_modules/originModuleDir/normalizedName + const {originModulePath} = context; const fromModuleParentIdx = originModulePath.lastIndexOf('node_modules' + path.sep) + 13; const originModuleDir = originModulePath.slice( 0, originModulePath.indexOf(path.sep, fromModuleParentIdx), ); - const absPath = path.join(originModuleDir, realModuleName); + const absPath = path.join(originModuleDir, normalizedName); return resolveModulePath(context, absPath, platform); } @@ -84,72 +88,64 @@ function resolve( // to allow overriding imports. It could be part of the custom resolver, but // that's not the case right now. if (context.allowHaste && !isDirectImport) { - const normalizedName = normalizePath(realModuleName); - const result = resolveHasteName(context, normalizedName, platform); - if (result.type === 'resolved') { - return result.resolution; + const modulePath = context.resolveHasteModule(normalizedName); + if (modulePath != null) { + return {type: 'sourceFile', filePath: modulePath}; } } if (resolveRequest) { try { - const resolution = resolveRequest(context, realModuleName, platform); + const resolution = resolveRequest(context, normalizedName, platform); if (resolution) { return resolution; } } catch (error) {} if (isDirectImport) { - throw new Error('Failed to resolve module: ' + realModuleName); + throw new Error('Failed to resolve module: ' + normalizedName); } } + const parsedName = parseModuleName(normalizedName); const modulePaths = []; - for (let modulePath of genModulePaths(context, realModuleName)) { - modulePath = context.redirectModulePath(modulePath); - + for (let packagePath of genPackagePaths(context, parsedName)) { + packagePath = context.redirectPackage(packagePath); + const modulePath = context.redirectModulePath( + path.join(packagePath, parsedName.file), + ); const result = resolveFileOrDir(context, modulePath, platform); if (result.type === 'resolved') { return result.resolution; } - modulePaths.push(modulePath); } throw new FailedToResolveNameError(modulePaths); } -/** Generate the potential module paths */ -function* genModulePaths( +function parseModuleName(moduleName: string): ModuleParts { + const parts = moduleName.split(path.sep); + const scope = parts[0].startsWith('@') ? parts[0] : ''; + return { + scope, + package: parts[scope ? 1 : 0], + file: parts.slice(scope ? 2 : 1).join(path.sep), + }; +} + +function* genPackagePaths( context: ResolutionContext, - toModuleName: string, + parsedName: ModuleParts, ): Iterable { - const {extraNodeModules, follow, originModulePath} = context; - - /** - * Extract the scope and package name from the module name. - */ - let bits = path.normalize(toModuleName).split(path.sep); - let packageName, scopeName; - if (bits.length >= 2 && bits[0].startsWith('@')) { - packageName = bits.slice(0, 2).join('/'); - scopeName = bits[0]; - bits = bits.slice(2); - } else { - packageName = bits.shift(); - } - /** * Find the nearest "node_modules" directory that contains * the imported package. */ - const {root} = path.parse(originModulePath); - let parent = originModulePath; + const {root} = path.parse(context.originModulePath); + let parent = context.originModulePath; do { parent = path.dirname(parent); if (path.basename(parent) !== 'node_modules') { - yield path.join( - follow(path.join(parent, 'node_modules', packageName)), - ...bits, - ); + yield path.join(parent, 'node_modules', parsedName.package); } } while (parent !== root); @@ -157,13 +153,13 @@ function* genModulePaths( * Check the user-provided `extraNodeModules` module map for a * direct mapping to a directory that contains the imported package. */ - if (extraNodeModules) { - parent = - extraNodeModules[packageName] || - (scopeName ? extraNodeModules[scopeName] : void 0); - - if (parent) { - yield path.join(follow(path.join(parent, packageName)), ...bits); + if (context.extraNodeModules) { + const extras = context.extraNodeModules; + if ((parent = extras[parsedName.package])) { + yield path.join(parent, parsedName.package); + } + if (parsedName.scope && (parent = extras[parsedName.scope])) { + yield path.join(parent, parsedName.package); } } } @@ -194,65 +190,6 @@ function resolveModulePath( throw new FailedToResolvePathError(result.candidates); } -/** - * Resolve a module as a Haste module or package. For example we might try to - * resolve `Foo`, that is provided by file `/smth/Foo.js`. Or, in the case of - * a Haste package, it could be `/smth/Foo/index.js`. - */ -function resolveHasteName( - context: HasteContext, - moduleName: string, - platform: string | null, -): Result { - const modulePath = context.resolveHasteModule(moduleName); - if (modulePath != null) { - return resolvedAs({type: 'sourceFile', filePath: modulePath}); - } - let packageName = moduleName; - let packageJsonPath = context.resolveHastePackage(packageName); - while (packageJsonPath == null && packageName && packageName !== '.') { - packageName = path.dirname(packageName); - packageJsonPath = context.resolveHastePackage(packageName); - } - if (packageJsonPath == null) { - return failedFor(); - } - const packageDirPath = path.dirname(packageJsonPath); - const pathInModule = moduleName.substring(packageName.length + 1); - const potentialModulePath = path.join(packageDirPath, pathInModule); - const result = resolveFileOrDir(context, potentialModulePath, platform); - if (result.type === 'resolved') { - return result; - } - const {candidates} = result; - const opts = {moduleName, packageName, pathInModule, candidates}; - throw new MissingFileInHastePackageError(opts); -} - -class MissingFileInHastePackageError extends Error { - candidates: FileAndDirCandidates; - moduleName: string; - packageName: string; - pathInModule: string; - - constructor(opts: {| - +candidates: FileAndDirCandidates, - +moduleName: string, - +packageName: string, - +pathInModule: string, - |}) { - super( - `While resolving module \`${opts.moduleName}\`, ` + - `the Haste package \`${opts.packageName}\` was found. However the ` + - `module \`${opts.pathInModule}\` could not be found within ` + - 'the package. Indeed, none of these files exist:\n\n' + - ` * \`${formatFileCandidates(opts.candidates.file)}\`\n` + - ` * \`${formatFileCandidates(opts.candidates.dir)}\``, - ); - Object.assign(this, opts); - } -} - /** * In the NodeJS-style module resolution scheme we want to check potential * paths both as directories and as files. For example, `/foo/bar` may resolve diff --git a/packages/metro-resolver/src/types.js b/packages/metro-resolver/src/types.js index 8c68a8f73d..a8cc1175fe 100644 --- a/packages/metro-resolver/src/types.js +++ b/packages/metro-resolver/src/types.js @@ -90,12 +90,6 @@ export type HasteContext = FileOrDirContext & { * a Haste module of that name. Ex. for `Foo` it may return `/smth/Foo.js`. */ +resolveHasteModule: (name: string) => ?string, - /** - * Given a name, this should return the full path to the package manifest that - * provides a Haste package of that name. Ex. for `Foo` it may return - * `/smth/Foo/package.json`. - */ - +resolveHastePackage: (name: string) => ?string, }; export type ModulePathContext = FileOrDirContext & { @@ -111,7 +105,7 @@ export type ResolutionContext = ModulePathContext & allowPnp: boolean, allowHaste: boolean, extraNodeModules: ?{[string]: string}, - originModulePath: string, + redirectPackage: (packagePath: string) => string, resolveRequest?: ?CustomResolver, follow: FollowFn, }; diff --git a/packages/metro/src/node-haste/DependencyGraph.js b/packages/metro/src/node-haste/DependencyGraph.js index b6b1433615..7ace0eebde 100644 --- a/packages/metro/src/node-haste/DependencyGraph.js +++ b/packages/metro/src/node-haste/DependencyGraph.js @@ -91,7 +91,7 @@ class DependencyGraph extends EventEmitter { resetCache: config.resetCache, rootDir: config.projectRoot, roots: config.watchFolders, - throwOnModuleCollision: true, + skipHastePackages: true, useWatchman: config.resolver.useWatchman, watch: true, }); diff --git a/packages/metro/src/node-haste/DependencyGraph/ModuleResolution.js b/packages/metro/src/node-haste/DependencyGraph/ModuleResolution.js index 28fda670f5..b5a300af1c 100644 --- a/packages/metro/src/node-haste/DependencyGraph/ModuleResolution.js +++ b/packages/metro/src/node-haste/DependencyGraph/ModuleResolution.js @@ -91,7 +91,7 @@ class ModuleResolver { const fromPackagePath = './' + path.relative( - path.dirname(fromPackage.path), + fromPackage.root, path.resolve(path.dirname(fromModule.path), modulePath), ); @@ -108,19 +108,19 @@ class ModuleResolver { './' + path.relative( path.dirname(fromModule.path), - path.resolve(path.dirname(fromPackage.path), redirectedPath), + path.resolve(fromPackage.root, redirectedPath), ); } return redirectedPath; } } else { - const pck = path.isAbsolute(modulePath) + const pack = path.isAbsolute(modulePath) ? moduleCache.getModule(modulePath).getPackage() : fromModule.getPackage(); - if (pck) { - return pck.redirectRequire(modulePath, this._options.mainFields); + if (pack) { + return pack.redirectRequire(modulePath, this._options.mainFields); } } } catch (err) { @@ -146,11 +146,21 @@ class ModuleResolver { this._redirectRequire(fromModule, modulePath), allowHaste, platform, - resolveHasteModule: name => - this._options.moduleMap.getModule(name, platform, true), - resolveHastePackage: name => - this._options.moduleMap.getPackage(name, platform, true), - getPackageMainPath: this._getPackageMainPath, + resolveHasteModule(name) { + return this.moduleMap.getModule(name, platform, true); + }, + getPackageMainPath(packageJsonPath: string): string { + return this.moduleCache + .getPackage(packageJsonPath) + .getMain(this.mainFields); + }, + redirectPackage(packagePath: string): string { + packagePath = this.follow(packagePath); + const packageJsonPath = path.join(packagePath, 'package.json'); + return this.doesFileExist(packageJsonPath) + ? this.moduleCache.getPackage(packageJsonPath).root + : packagePath; + }, }, moduleName, platform, @@ -194,11 +204,6 @@ class ModuleResolver { } } - _getPackageMainPath = (packageJsonPath: string): string => { - const package_ = this._options.moduleCache.getPackage(packageJsonPath); - return package_.getMain(this._options.mainFields); - }; - /** * FIXME: get rid of this function and of the reliance on `TModule` * altogether, return strongly typed resolutions at the top-level instead. diff --git a/packages/metro/src/node-haste/DependencyGraph/ResolutionRequest.js b/packages/metro/src/node-haste/DependencyGraph/ResolutionRequest.js index bcff03f39a..0dd2d38240 100644 --- a/packages/metro/src/node-haste/DependencyGraph/ResolutionRequest.js +++ b/packages/metro/src/node-haste/DependencyGraph/ResolutionRequest.js @@ -22,6 +22,7 @@ import type {ModuleResolver} from './ModuleResolution'; export type Packageish = { path: string, + root: string, redirectRequire( toModuleName: string, mainFields: $ReadOnlyArray, diff --git a/packages/metro/src/node-haste/ModuleCache.js b/packages/metro/src/node-haste/ModuleCache.js index bd17f12fc0..0c90165c7c 100644 --- a/packages/metro/src/node-haste/ModuleCache.js +++ b/packages/metro/src/node-haste/ModuleCache.js @@ -13,18 +13,22 @@ const Module = require('./Module'); const Package = require('./Package'); +import type {PackageContent} from './Package'; + type GetClosestPackageFn = (filePath: string) => ?string; class ModuleCache { _getClosestPackage: GetClosestPackageFn; _moduleCache: {[filePath: string]: Module, __proto__: null}; _packageCache: {[filePath: string]: Package, __proto__: null}; + _packagesById: {[id: string]: Package, __proto__: null}; _packageModuleMap: WeakMap; constructor(options: {getClosestPackage: GetClosestPackageFn}) { this._getClosestPackage = options.getClosestPackage; this._moduleCache = Object.create(null); this._packageCache = Object.create(null); + this._packagesById = Object.create(null); this._packageModuleMap = new WeakMap(); } @@ -36,12 +40,19 @@ class ModuleCache { } getPackage(filePath: string): Package { - if (!this._packageCache[filePath]) { - this._packageCache[filePath] = new Package({ + const cachedPackage = this._packageCache[filePath]; + if (!cachedPackage) { + const newPackage = new Package({ file: filePath, }); + this._packageCache[filePath] = newPackage; + const packageId = getPackageId(newPackage.read()); + return ( + this._packagesById[packageId] || + (this._packagesById[packageId] = newPackage) + ); } - return this._packageCache[filePath]; + return cachedPackage; } getPackageForModule(module: Module): ?Package { @@ -59,8 +70,9 @@ class ModuleCache { return null; } - this._packageModuleMap.set(module, packagePath); - return this.getPackage(packagePath); + const pack = this.getPackage(packagePath); + this._packageModuleMap.set(module, pack.path); + return pack; } processFileChange(type: string, filePath: string) { @@ -68,11 +80,20 @@ class ModuleCache { this._moduleCache[filePath].invalidate(); delete this._moduleCache[filePath]; } - if (this._packageCache[filePath]) { - this._packageCache[filePath].invalidate(); + const pack = this._packageCache[filePath]; + if (pack) { + if (pack.content) { + const packageId = getPackageId(pack.content); + delete this._packagesById[packageId]; + } + pack.invalidate(); delete this._packageCache[filePath]; } } } +function getPackageId(content: PackageContent) { + return content.name + '@' + content.version; +} + module.exports = ModuleCache; diff --git a/packages/metro/src/node-haste/Package.js b/packages/metro/src/node-haste/Package.js index 6660dd8c00..57e179c02e 100644 --- a/packages/metro/src/node-haste/Package.js +++ b/packages/metro/src/node-haste/Package.js @@ -13,8 +13,9 @@ const fs = require('fs'); const path = require('path'); -type PackageContent = { +export type PackageContent = { name: string, + version: string, 'react-native': mixed, browser: mixed, main: ?string, @@ -22,14 +23,13 @@ type PackageContent = { class Package { path: string; - - _root: string; - _content: ?PackageContent; + root: string; + content: ?PackageContent; constructor({file}: {file: string}) { this.path = path.resolve(file); - this._root = path.dirname(this.path); - this._content = null; + this.root = path.dirname(this.path); + this.content = null; } /** @@ -76,11 +76,11 @@ class Package { } /* $FlowFixMe: `getReplacements` doesn't validate the return value. */ - return path.join(this._root, main); + return path.join(this.root, main); } invalidate() { - this._content = null; + this.content = null; } redirectRequire( @@ -101,7 +101,7 @@ class Package { } let relPath = - './' + path.relative(this._root, path.resolve(this._root, name)); + './' + path.relative(this.root, path.resolve(this.root, name)); if (path.sep !== '/') { relPath = relPath.replace(new RegExp('\\' + path.sep, 'g'), '/'); @@ -123,17 +123,17 @@ class Package { } if (redirect) { - return path.join(this._root, redirect); + return path.join(this.root, redirect); } return name; } read(): PackageContent { - if (this._content == null) { - this._content = JSON.parse(fs.readFileSync(this.path, 'utf8')); + if (this.content == null) { + this.content = JSON.parse(fs.readFileSync(this.path, 'utf8')); } - return this._content; + return this.content; } }