From 9293e54085fc834778e1d25746e9d1875088e30d Mon Sep 17 00:00:00 2001 From: Amjad Masad Date: Fri, 25 Sep 2015 00:17:15 -0700 Subject: [PATCH] Warn and randomize colliding name selection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: @​public The issue of colliding haste modules have came up many times and have wasted countless engineering hours. This will start warning about it and will also start selecting modules at random so that people don't depend on undefined behavior. Additionally, this surfaced an issue where with assets we may fatally throw if the directory doesn't exist. This is fixed by checking the existence of the directory before trying to match files in it. Reviewed By: @jingc Differential Revision: D2478480 --- .../DependencyGraph/HasteMap.js | 45 ++++++++++++++----- .../DependencyGraph/ResolutionRequest.js | 7 ++- .../src/lib/getPlatformExtension.js | 2 +- 3 files changed, 42 insertions(+), 12 deletions(-) diff --git a/packager/react-packager/src/DependencyResolver/DependencyGraph/HasteMap.js b/packager/react-packager/src/DependencyResolver/DependencyGraph/HasteMap.js index 576ea3e03..0b87cd1eb 100644 --- a/packager/react-packager/src/DependencyResolver/DependencyGraph/HasteMap.js +++ b/packager/react-packager/src/DependencyResolver/DependencyGraph/HasteMap.js @@ -8,8 +8,9 @@ */ 'use strict'; +const chalk = require('chalk'); const path = require('path'); -const getPontentialPlatformExt = require('../../lib/getPlatformExtension'); +const getPlatformExtension = require('../../lib/getPlatformExtension'); class HasteMap { constructor({ fastfs, moduleCache, helpers }) { @@ -17,6 +18,7 @@ class HasteMap { this._moduleCache = moduleCache; this._helpers = helpers; this._map = Object.create(null); + this._warnedAbout = Object.create(null); } build() { @@ -35,6 +37,9 @@ class HasteMap { processFileChange(type, absPath) { return Promise.resolve().then(() => { + // Rewarn after file changes. + this._warnedAbout = Object.create(null); + /*eslint no-labels: 0 */ if (type === 'delete' || type === 'change') { loop: for (let name in this._map) { @@ -64,19 +69,39 @@ class HasteMap { } getModule(name, platform = null) { - if (this._map[name]) { - const modules = this._map[name]; - if (platform != null) { - for (let i = 0; i < modules.length; i++) { - if (getPontentialPlatformExt(modules[i].path) === platform) { - return modules[i]; - } + if (!this._map[name]) { + return null; + } + + const modules = this._map[name]; + if (platform != null) { + for (let i = 0; i < modules.length; i++) { + if (getPlatformExtension(modules[i].path) === platform) { + return modules[i]; } } - return modules[0]; + if (modules.length > 1) { + if (!this._warnedAbout[name]) { + this._warnedAbout[name] = true; + console.warn( + chalk.yellow( + '\nWARNING: Found multiple haste modules or packages ' + + 'with the name `%s`. Please fix this by adding it to ' + + 'the blacklist or deleting the modules keeping only one.\n' + + 'One of the following modules will be selected at random:\n%s\n' + ), + name, + modules.map(m => m.path).join('\n'), + ); + } + + const randomIndex = Math.floor(Math.random() * modules.length); + return modules[randomIndex]; + } } - return null; + + return modules[0]; } _processHasteModule(file) { diff --git a/packager/react-packager/src/DependencyResolver/DependencyGraph/ResolutionRequest.js b/packager/react-packager/src/DependencyResolver/DependencyGraph/ResolutionRequest.js index 3be6abd91..2ed0dc9f2 100644 --- a/packager/react-packager/src/DependencyResolver/DependencyGraph/ResolutionRequest.js +++ b/packager/react-packager/src/DependencyResolver/DependencyGraph/ResolutionRequest.js @@ -252,6 +252,11 @@ class ResolutionRequest { _loadAsFile(potentialModulePath) { return Promise.resolve().then(() => { if (this._helpers.isAssetFile(potentialModulePath)) { + const dirname = path.dirname(potentialModulePath); + if (!this._fastfs.dirExists(dirname)) { + throw new UnableToResolveError(`Directory ${dirname} doesn't exist`); + } + const {name, type} = getAssetDataFromName(potentialModulePath); let pattern = '^' + name + '(@[\\d\\.]+x)?'; @@ -263,7 +268,7 @@ class ResolutionRequest { // We arbitrarly grab the first one, because scale selection // will happen somewhere const [assetFile] = this._fastfs.matches( - path.dirname(potentialModulePath), + dirname, new RegExp(pattern) ); diff --git a/packager/react-packager/src/lib/getPlatformExtension.js b/packager/react-packager/src/lib/getPlatformExtension.js index 3f34da421..417cd866c 100644 --- a/packager/react-packager/src/lib/getPlatformExtension.js +++ b/packager/react-packager/src/lib/getPlatformExtension.js @@ -10,7 +10,7 @@ const path = require('path'); -const SUPPORTED_PLATFORM_EXTS = ['android', 'ios']; +const SUPPORTED_PLATFORM_EXTS = ['android', 'ios', 'web']; const re = new RegExp( '[^\\.]+\\.(' + SUPPORTED_PLATFORM_EXTS.join('|') + ')\\.\\w+$'