From 502d277ff2a6bc6d589865e35e47998390dccf7d Mon Sep 17 00:00:00 2001 From: Amjad Masad Date: Wed, 30 Sep 2015 19:02:52 -0700 Subject: [PATCH] Error on name collisions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: @​public This moves us from warnings on name collisions to errors. If the error happens in initialization it will fatal out. However, if the error happens while working (after initialization) then I did my best to make it recoverable. The rational behind this is that if you're working and you're changing names, you may introduce a duplication while moving things around. It will suck if you have to restart the server every time you do that. Reviewed By: @frantic Differential Revision: D2493098 --- .../DependencyGraph/DeprecatedAssetMap.js | 1 + .../DependencyGraph/HasteMap.js | 61 +++----- .../DependencyGraph/ResolutionRequest.js | 1 + .../__tests__/DependencyGraph-test.js | 136 ++---------------- .../DependencyGraph/index.js | 28 +++- 5 files changed, 58 insertions(+), 169 deletions(-) diff --git a/packager/react-packager/src/DependencyResolver/DependencyGraph/DeprecatedAssetMap.js b/packager/react-packager/src/DependencyResolver/DependencyGraph/DeprecatedAssetMap.js index ca537078f..7292a85ce 100644 --- a/packager/react-packager/src/DependencyResolver/DependencyGraph/DeprecatedAssetMap.js +++ b/packager/react-packager/src/DependencyResolver/DependencyGraph/DeprecatedAssetMap.js @@ -13,6 +13,7 @@ const AssetModule_DEPRECATED = require('../AssetModule_DEPRECATED'); const Fastfs = require('../fastfs'); const debug = require('debug')('ReactNativePackager:DependencyGraph'); const path = require('path'); +const Promise = require('promise'); class DeprecatedAssetMap { constructor({ fsCrawl, roots, assetExts, fileWatcher, ignoreFilePath, helpers }) { diff --git a/packager/react-packager/src/DependencyResolver/DependencyGraph/HasteMap.js b/packager/react-packager/src/DependencyResolver/DependencyGraph/HasteMap.js index 6d720ea50..6c8bdc0a4 100644 --- a/packager/react-packager/src/DependencyResolver/DependencyGraph/HasteMap.js +++ b/packager/react-packager/src/DependencyResolver/DependencyGraph/HasteMap.js @@ -8,9 +8,9 @@ */ 'use strict'; -const chalk = require('chalk'); const path = require('path'); const getPlatformExtension = require('../../lib/getPlatformExtension'); +const Promise = require('promise'); const GENERIC_PLATFORM = 'generic'; @@ -19,11 +19,11 @@ class HasteMap { this._fastfs = fastfs; this._moduleCache = moduleCache; this._helpers = helpers; - this._map = Object.create(null); - this._warnedAbout = Object.create(null); } build() { + this._map = Object.create(null); + let promises = this._fastfs.findFilesByExt('js', { ignore: (file) => this._helpers.isNodeModulesDir(file) }).map(file => this._processHasteModule(file)); @@ -39,20 +39,15 @@ 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) { 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; - } + const module = modulesMap[platform]; + if (module.path === absPath) { + delete modulesMap[platform]; + break loop; } } } @@ -82,38 +77,15 @@ class HasteMap { // 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]; + return modulesMap[GENERIC_PLATFORM]; } else { - modules = modulesMap[platform]; - if (modules == null) { - modules = modulesMap[GENERIC_PLATFORM]; + let module = modulesMap[platform]; + if (module == null) { + module = modulesMap[GENERIC_PLATFORM]; } + return module; } - - 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' - ), - name, - modules.map(m => m.path).join('\n'), - ); - } - - return modules[0]; - } - - return modules[0]; } _processHasteModule(file) { @@ -147,11 +119,14 @@ class HasteMap { const moduleMap = this._map[name]; const modulePlatform = getPlatformExtension(mod.path) || GENERIC_PLATFORM; - if (!moduleMap[modulePlatform]) { - moduleMap[modulePlatform] = []; + if (moduleMap[modulePlatform]) { + throw new Error( + `Naming collision detected: ${mod.path} ` + + `collides with ${moduleMap[modulePlatform].path}` + ); } - moduleMap[modulePlatform].push(mod); + moduleMap[modulePlatform] = mod; } } diff --git a/packager/react-packager/src/DependencyResolver/DependencyGraph/ResolutionRequest.js b/packager/react-packager/src/DependencyResolver/DependencyGraph/ResolutionRequest.js index f3a755375..dda09abe8 100644 --- a/packager/react-packager/src/DependencyResolver/DependencyGraph/ResolutionRequest.js +++ b/packager/react-packager/src/DependencyResolver/DependencyGraph/ResolutionRequest.js @@ -13,6 +13,7 @@ const util = require('util'); const path = require('path'); const isAbsolutePath = require('absolute-path'); const getAssetDataFromName = require('../../lib/getAssetDataFromName'); +const Promise = require('promise'); class ResolutionRequest { constructor({ diff --git a/packager/react-packager/src/DependencyResolver/DependencyGraph/__tests__/DependencyGraph-test.js b/packager/react-packager/src/DependencyResolver/DependencyGraph/__tests__/DependencyGraph-test.js index 017ffe8e7..1a9350fa5 100644 --- a/packager/react-packager/src/DependencyResolver/DependencyGraph/__tests__/DependencyGraph-test.js +++ b/packager/react-packager/src/DependencyResolver/DependencyGraph/__tests__/DependencyGraph-test.js @@ -1070,7 +1070,7 @@ describe('DependencyGraph', function() { }); }); - pit('can have multiple modules with the same name', function() { + pit('should fatal on multiple modules with the same name', function() { var root = '/root'; fs.__setMockFilesystem({ 'root': { @@ -1078,59 +1078,33 @@ describe('DependencyGraph', function() { '/**', ' * @providesModule index', ' */', - 'require("b")', ].join('\n'), 'b.js': [ '/**', - ' * @providesModule b', + ' * @providesModule index', ' */', ].join('\n'), - 'c.js': [ - '/**', - ' * @providesModule c', - ' */', - ].join('\n'), - 'somedir': { - 'somefile.js': [ - '/**', - ' * @providesModule index', - ' */', - 'require("c")', - ].join('\n') - } } }); + const _exit = process.exit; + const _error = console.error; + + process.exit = jest.genMockFn(); + console.error = jest.genMockFn(); + var dgraph = new DependencyGraph({ roots: [root], fileWatcher: fileWatcher, assetExts: ['png', 'jpg'], cache: cache, }); - return getOrderedDependenciesAsJSON(dgraph, '/root/somedir/somefile.js').then(function(deps) { - expect(deps) - .toEqual([ - { - id: 'index', - path: '/root/somedir/somefile.js', - dependencies: ['c'], - isAsset: false, - isAsset_DEPRECATED: false, - isJSON: false, - isPolyfill: false, - resolution: undefined, - }, - { - id: 'c', - path: '/root/c.js', - dependencies: [], - isAsset: false, - isAsset_DEPRECATED: false, - isJSON: false, - isPolyfill: false, - resolution: undefined, - }, - ]); + + return dgraph.load().catch(() => { + expect(process.exit).toBeCalledWith(1); + expect(console.error).toBeCalled(); + process.exit = _exit; + console.error = _error; }); }); @@ -3629,88 +3603,6 @@ 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'; diff --git a/packager/react-packager/src/DependencyResolver/DependencyGraph/index.js b/packager/react-packager/src/DependencyResolver/DependencyGraph/index.js index a03cd3dbe..1baef52f3 100644 --- a/packager/react-packager/src/DependencyResolver/DependencyGraph/index.js +++ b/packager/react-packager/src/DependencyResolver/DependencyGraph/index.js @@ -73,7 +73,11 @@ class DependencyGraph { this._opts = validateOpts(options); this._cache = this._opts.cache; this._helpers = new Helpers(this._opts); - this.load(); + this.load().catch((err) => { + // This only happens at initialization. Live errors are easier to recover from. + console.error('Error building DepdendencyGraph:\n', err.stack); + process.exit(1); + }); } load() { @@ -198,9 +202,25 @@ class DependencyGraph { return; } - this._loading = this._loading.then( - () => this._hasteMap.processFileChange(type, absPath) - ); + // Ok, this is some tricky promise code. Our requirements are: + // * we need to report back failures + // * failures shouldn't block recovery + // * Errors can leave `hasteMap` in an incorrect state, and we need to rebuild + // After we process a file change we record any errors which will also be + // reported via the next request. On the next file change, we'll see that + // we are in an error state and we should decide to do a full rebuild. + this._loading = this._loading.finally(() => { + if (this._hasteMapError) { + this._hasteMapError = null; + // Rebuild the entire map if last change resulted in an error. + console.warn('Rebuilding haste map to recover from error'); + this._loading = this._hasteMap.build(); + } else { + this._loading = this._hasteMap.processFileChange(type, absPath); + this._loading.catch((e) => this._hasteMapError = e); + } + return this._loading; + }); } }