From 2a1d95825133eae8124d0aff0519907379af85b4 Mon Sep 17 00:00:00 2001 From: Amjad Masad Date: Fri, 25 Sep 2015 20:17:24 -0700 Subject: [PATCH] Fix haste resolution (and better warnings) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: @​public Fix the haste resolution algorithm. Changed the map data structure from a list of modules, to a list of modules grouped by platform. This considerably simplifies the "getModule" method and makes it easy to properly warn about colliding module names. This also fixes a bug where we used to include `.web` files when we shouldn't (see task). Reviewed By: @vjeux Differential Revision: D2482969 --- .../DependencyGraph/HasteMap.js | 88 ++++++----- .../__tests__/DependencyGraph-test.js | 147 +++++++++++------- 2 files changed, 139 insertions(+), 96 deletions(-) diff --git a/react-packager/src/DependencyResolver/DependencyGraph/HasteMap.js b/react-packager/src/DependencyResolver/DependencyGraph/HasteMap.js index 0b87cd1e..386651cf 100644 --- a/react-packager/src/DependencyResolver/DependencyGraph/HasteMap.js +++ b/react-packager/src/DependencyResolver/DependencyGraph/HasteMap.js @@ -12,6 +12,8 @@ const chalk = require('chalk'); const path = require('path'); const getPlatformExtension = require('../../lib/getPlatformExtension'); +const GENERIC_PLATFORM = 'generic'; + class HasteMap { constructor({ fastfs, moduleCache, helpers }) { this._fastfs = fastfs; @@ -43,11 +45,14 @@ class HasteMap { /*eslint no-labels: 0 */ if (type === 'delete' || type === 'change') { loop: for (let name in this._map) { - let modules = this._map[name]; - for (var i = 0; i < modules.length; i++) { - if (modules[i].path === absPath) { - modules.splice(i, 1); - break loop; + const modulesMap = this._map[name]; + for (let platform in modulesMap) { + const modules = modulesMap[platform]; + for (var i = 0; i < modules.length; i++) { + if (modules[i].path === absPath) { + modules.splice(i, 1); + break loop; + } } } } @@ -69,36 +74,45 @@ class HasteMap { } getModule(name, platform = null) { - if (!this._map[name]) { + const modulesMap = this._map[name]; + if (modulesMap == null) { 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]; - } + // If no platform is given we choose the generic platform module list. + // If a platform is given and no modules exist we fallback + // to the generic platform module list. + let modules; + if (platform == null) { + modules = modulesMap[GENERIC_PLATFORM]; + } else { + modules = modulesMap[platform]; + if (modules == null) { + modules = modulesMap[GENERIC_PLATFORM]; + } + } + + if (modules == null) { + return null; + } + + 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'), + ); } - 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]; - } + const randomIndex = Math.floor(Math.random() * modules.length); + return modules[randomIndex]; } return modules[0]; @@ -129,15 +143,17 @@ class HasteMap { _updateHasteMap(name, mod) { if (this._map[name] == null) { - this._map[name] = []; + this._map[name] = Object.create(null); } - if (mod.type === 'Module') { - // Modules takes precendence over packages. - this._map[name].unshift(mod); - } else { - this._map[name].push(mod); + const moduleMap = this._map[name]; + const modulePlatform = getPlatformExtension(mod.path) || GENERIC_PLATFORM; + + if (!moduleMap[modulePlatform]) { + moduleMap[modulePlatform] = []; } + + moduleMap[modulePlatform].push(mod); } } diff --git a/react-packager/src/DependencyResolver/DependencyGraph/__tests__/DependencyGraph-test.js b/react-packager/src/DependencyResolver/DependencyGraph/__tests__/DependencyGraph-test.js index 06f65127..017ffe8e 100644 --- a/react-packager/src/DependencyResolver/DependencyGraph/__tests__/DependencyGraph-test.js +++ b/react-packager/src/DependencyResolver/DependencyGraph/__tests__/DependencyGraph-test.js @@ -1134,66 +1134,6 @@ describe('DependencyGraph', function() { }); }); - pit('providesModule wins when conflict with package', function() { - var root = '/root'; - fs.__setMockFilesystem({ - 'root': { - 'index.js': [ - '/**', - ' * @providesModule index', - ' */', - 'require("aPackage")', - ].join('\n'), - 'b.js': [ - '/**', - ' * @providesModule aPackage', - ' */', - ].join('\n'), - 'aPackage': { - 'package.json': JSON.stringify({ - name: 'aPackage', - main: 'main.js' - }), - 'main.js': 'lol' - } - } - }); - - var dgraph = new DependencyGraph({ - roots: [root], - fileWatcher: fileWatcher, - assetExts: ['png', 'jpg'], - cache: cache, - }); - return getOrderedDependenciesAsJSON(dgraph, '/root/index.js').then(function(deps) { - expect(deps) - .toEqual([ - { - id: 'index', - path: '/root/index.js', - dependencies: ['aPackage'], - isAsset: false, - isAsset_DEPRECATED: false, - isJSON: false, - isPolyfill: false, - resolution: undefined, - resolveDependency: undefined, - }, - { - id: 'aPackage', - path: '/root/b.js', - dependencies: [], - isAsset: false, - isAsset_DEPRECATED: false, - isJSON: false, - isPolyfill: false, - resolution: undefined, - resolveDependency: undefined, - }, - ]); - }); - }); - pit('should be forgiving with missing requires', function() { var root = '/root'; fs.__setMockFilesystem({ @@ -2616,6 +2556,11 @@ describe('DependencyGraph', function() { * @providesModule a */ `, + 'a.web.js': ` + /** + * @providesModule a + */ + `, } }); @@ -3684,6 +3629,88 @@ describe('DependencyGraph', function() { }); }); + describe('warnings', () => { + let warn = console.warn; + + beforeEach(() => { + console.warn = jest.genMockFn(); + }); + + afterEach(() => { + console.warn = warn; + }); + + pit('should warn about colliding module names', function() { + fs.__setMockFilesystem({ + 'root': { + 'index.js': ` + /** + * @providesModule index + */ + require('a'); + `, + 'a.js': ` + /** + * @providesModule a + */ + `, + 'b.js': ` + /** + * @providesModule a + */ + `, + } + }); + + var dgraph = new DependencyGraph({ + roots: ['/root'], + fileWatcher: fileWatcher, + assetExts: ['png', 'jpg'], + cache: cache, + }); + + return getOrderedDependenciesAsJSON(dgraph, '/root/index.js').then(function(deps) { + expect(console.warn.mock.calls.length).toBe(1); + }); + }); + + + pit('should warn about colliding module names within a platform', function() { + var root = '/root'; + fs.__setMockFilesystem({ + 'root': { + 'index.ios.js': ` + /** + * @providesModule index + */ + require('a'); + `, + 'a.ios.js': ` + /** + * @providesModule a + */ + `, + 'b.ios.js': ` + /** + * @providesModule a + */ + `, + } + }); + + var dgraph = new DependencyGraph({ + roots: [root], + fileWatcher: fileWatcher, + assetExts: ['png', 'jpg'], + cache: cache, + }); + + return getOrderedDependenciesAsJSON(dgraph, '/root/index.ios.js', 'ios').then(function(deps) { + expect(console.warn.mock.calls.length).toBe(1); + }); + }); + }); + describe('getAsyncDependencies', () => { pit('should get dependencies', function() { var root = '/root';