From 17ccb9b8c4b39cf6460325952933489e02757f02 Mon Sep 17 00:00:00 2001 From: Christoph Pojer Date: Wed, 30 Nov 2016 10:10:34 -0800 Subject: [PATCH] kill `shouldThrowOnUnresolvedErrors` option Summary: Removes the `shouldThrowOnUnresolvedErrors` option, as now it is only ever `() => true` Reviewed By: davidaurelio Differential Revision: D4237711 fbshipit-source-id: 9460f0f0c50dc0d08d17cb7bdeb995825f7051f3 --- .../src/ModuleGraph/node-haste/node-haste.js | 2 - react-packager/src/Resolver/index.js | 1 - .../DependencyGraph/ResolutionRequest.js | 47 +-- .../__tests__/DependencyGraph-test.js | 342 ++++++++---------- react-packager/src/node-haste/index.js | 2 - 5 files changed, 172 insertions(+), 222 deletions(-) diff --git a/react-packager/src/ModuleGraph/node-haste/node-haste.js b/react-packager/src/ModuleGraph/node-haste/node-haste.js index 426f1e24..1c41fed9 100644 --- a/react-packager/src/ModuleGraph/node-haste/node-haste.js +++ b/react-packager/src/ModuleGraph/node-haste/node-haste.js @@ -37,7 +37,6 @@ type ResolveOptions = {| |}; const platforms = new Set(defaults.platforms); -const returnTrue = () => true; exports.createResolveFn = function(options: ResolveOptions): ResolveFn { const { @@ -87,7 +86,6 @@ exports.createResolveFn = function(options: ResolveOptions): ResolveFn { platform, platforms, preferNativePlatform: true, - shouldThrowOnUnresolvedErrors: returnTrue, }); } diff --git a/react-packager/src/Resolver/index.js b/react-packager/src/Resolver/index.js index f4cf6583..00fb8354 100644 --- a/react-packager/src/Resolver/index.js +++ b/react-packager/src/Resolver/index.js @@ -98,7 +98,6 @@ class Resolver { preferNativePlatform: true, watch: opts.watch, cache: opts.cache, - shouldThrowOnUnresolvedErrors: () => true, transformCode: opts.transformCode, transformCacheKey: opts.transformCacheKey, extraNodeModules: opts.extraNodeModules, diff --git a/react-packager/src/node-haste/DependencyGraph/ResolutionRequest.js b/react-packager/src/node-haste/DependencyGraph/ResolutionRequest.js index 7da2d056..ad683953 100644 --- a/react-packager/src/node-haste/DependencyGraph/ResolutionRequest.js +++ b/react-packager/src/node-haste/DependencyGraph/ResolutionRequest.js @@ -27,8 +27,6 @@ import type Module from '../Module'; import type ModuleCache from '../ModuleCache'; import type ResolutionResponse from './ResolutionResponse'; -const emptyModule = require.resolve('./assets/empty-module.js'); - type DirExistsFn = (filePath: string) => boolean; type Options = { @@ -43,7 +41,6 @@ type Options = { platform: string, platforms: Set, preferNativePlatform: boolean, - shouldThrowOnUnresolvedErrors: () => boolean, }; class ResolutionRequest { @@ -58,7 +55,7 @@ class ResolutionRequest { _platform: string; _platforms: Set; _preferNativePlatform: boolean; - _shouldThrowOnUnresolvedErrors: () => boolean; + static emptyModule: string; constructor({ dirExists, @@ -71,7 +68,6 @@ class ResolutionRequest { platform, platforms, preferNativePlatform, - shouldThrowOnUnresolvedErrors, }: Options) { this._dirExists = dirExists; this._entryPath = entryPath; @@ -83,7 +79,6 @@ class ResolutionRequest { this._platform = platform; this._platforms = platforms; this._preferNativePlatform = preferNativePlatform; - this._shouldThrowOnUnresolvedErrors = shouldThrowOnUnresolvedErrors; this._resetResolutionCache(); } @@ -109,38 +104,16 @@ class ResolutionRequest { return result; }; - const forgive = (error) => { - if ( - error.type !== 'UnableToResolveError' || - this._shouldThrowOnUnresolvedErrors(this._entryPath, this._platform) - ) { - throw error; - } - - debug( - 'Unable to resolve module %s from %s', - toModuleName, - fromModule.path - ); - return null; - }; - if (!this._helpers.isNodeModulesDir(fromModule.path) && !(isRelativeImport(toModuleName) || isAbsolutePath(toModuleName))) { return this._tryResolve( () => this._resolveHasteDependency(fromModule, toModuleName), () => this._resolveNodeDependency(fromModule, toModuleName) - ).then( - cacheResult, - forgive, - ); + ).then(cacheResult); } return this._resolveNodeDependency(fromModule, toModuleName) - .then( - cacheResult, - forgive, - ); + .then(cacheResult); } getOrderedDependencies({ @@ -311,7 +284,11 @@ class ResolutionRequest { return this._redirectRequire(fromModule, potentialModulePath).then( realModuleName => { if (realModuleName === false) { - return this._loadAsFile(emptyModule, fromModule, toModuleName); + return this._loadAsFile( + ResolutionRequest.emptyModule, + fromModule, + toModuleName, + ); } return this._tryResolve( @@ -330,7 +307,11 @@ class ResolutionRequest { realModuleName => { // exclude if (realModuleName === false) { - return this._loadAsFile(emptyModule, fromModule, toModuleName); + return this._loadAsFile( + ResolutionRequest.emptyModule, + fromModule, + toModuleName, + ); } if (isRelativeImport(realModuleName) || isAbsolutePath(realModuleName)) { @@ -526,4 +507,6 @@ function isRelativeImport(filePath) { return /^[.][.]?(?:[/]|$)/.test(filePath); } +ResolutionRequest.emptyModule = require.resolve('./assets/empty-module.js'); + module.exports = ResolutionRequest; diff --git a/react-packager/src/node-haste/__tests__/DependencyGraph-test.js b/react-packager/src/node-haste/__tests__/DependencyGraph-test.js index d871446d..ecb5c19b 100644 --- a/react-packager/src/node-haste/__tests__/DependencyGraph-test.js +++ b/react-packager/src/node-haste/__tests__/DependencyGraph-test.js @@ -26,6 +26,11 @@ jest.mock('../../JSTransformer/worker/extract-dependencies', () => extractDepend jasmine.DEFAULT_TIMEOUT_INTERVAL = 10000; const path = require('path'); + +const mockStat = { + isDirectory: () => false, +}; + beforeEach(() => { jest.resetModules(); @@ -34,6 +39,7 @@ beforeEach(() => { describe('DependencyGraph', function() { let Module; + let ResolutionRequest; let defaults; function getOrderedDependenciesAsJSON(dgraph, entryPath, platform, recursive = true) { @@ -58,6 +64,7 @@ describe('DependencyGraph', function() { jest.resetModules(); Module = require('../Module'); + ResolutionRequest = require('../DependencyGraph/ResolutionRequest'); const Cache = jest.genMockFn().mockImplementation(function() { this._maps = Object.create(null); @@ -107,7 +114,6 @@ describe('DependencyGraph', function() { 'react-native', ], platforms: ['ios', 'android'], - shouldThrowOnUnresolvedErrors: () => false, useWatchman: false, maxWorkers: 1, resetCache: true, @@ -1151,7 +1157,7 @@ describe('DependencyGraph', function() { }); }); - it('should be forgiving with missing requires', function() { + it('throws when a module is missing', function() { var root = '/root'; setMockFileSystem({ 'root': { @@ -1168,21 +1174,11 @@ describe('DependencyGraph', function() { ...defaults, roots: [root], }); - return getOrderedDependenciesAsJSON(dgraph, '/root/index.js').then(function(deps) { - expect(deps) - .toEqual([ - { - id: 'index', - path: '/root/index.js', - dependencies: ['lolomg'], - isAsset: false, - isJSON: false, - isPolyfill: false, - resolution: undefined, - resolveDependency: undefined, - }, - ]); - }); + return getOrderedDependenciesAsJSON(dgraph, '/root/index.js').catch( + error => { + expect(error.type).toEqual('UnableToResolveError'); + } + ); }); it('should work with packages with subdirs', function() { @@ -1908,9 +1904,11 @@ describe('DependencyGraph', function() { }); it('should support browser exclude of a package ("' + fieldName + '")', function() { + ResolutionRequest.emptyModule = '/root/emptyModule.js'; var root = '/root'; setMockFileSystem({ 'root': { + 'emptyModule.js': '', 'index.js': [ '/**', ' * @providesModule index', @@ -1958,14 +1956,26 @@ describe('DependencyGraph', function() { isPolyfill: false, resolution: undefined, }, + { + dependencies: [], + id: '/root/emptyModule.js', + isAsset: false, + isJSON: false, + isPolyfill: false, + path: '/root/emptyModule.js', + resolution: undefined, + }, ]); }); }); it('should support browser exclude of a file ("' + fieldName + '")', function() { + ResolutionRequest.emptyModule = '/root/emptyModule.js'; + var root = '/root'; setMockFileSystem({ 'root': { + 'emptyModule.js': '', 'index.js': [ '/**', ' * @providesModule index', @@ -1992,7 +2002,8 @@ describe('DependencyGraph', function() { return getOrderedDependenciesAsJSON(dgraph, '/root/index.js').then(function(deps) { expect(deps) .toEqual([ - { id: 'index', + { + id: 'index', path: '/root/index.js', dependencies: ['aPackage'], isAsset: false, @@ -2000,7 +2011,8 @@ describe('DependencyGraph', function() { isPolyfill: false, resolution: undefined, }, - { id: 'aPackage/index.js', + { + id: 'aPackage/index.js', path: '/root/aPackage/index.js', dependencies: ['./booga'], isAsset: false, @@ -2008,6 +2020,15 @@ describe('DependencyGraph', function() { isPolyfill: false, resolution: undefined, }, + { + dependencies: [], + id: '/root/emptyModule.js', + isAsset: false, + isJSON: false, + isPolyfill: false, + path: '/root/emptyModule.js', + resolution: undefined, + }, ]); }); }); @@ -2105,8 +2126,8 @@ describe('DependencyGraph', function() { const root = '/root'; setMockFileSystem({ [root.slice(1)]: { - 'index.js': 'require("/root/arbitrary.js");', - 'arbitrary.js': '', + 'index.js': 'require("/root/apple.js");', + 'apple.js': '', }, }); @@ -2120,15 +2141,15 @@ describe('DependencyGraph', function() { { id: '/root/index.js', path: '/root/index.js', - dependencies: ['/root/arbitrary.js'], + dependencies: ['/root/apple.js'], isAsset: false, isJSON: false, isPolyfill: false, resolution: undefined, }, { - id: '/root/arbitrary.js', - path: '/root/arbitrary.js', + id: '/root/apple.js', + path: '/root/apple.js', dependencies: [], isAsset: false, isJSON: false, @@ -2508,8 +2529,8 @@ describe('DependencyGraph', function() { const root = 'C:\\root'; setMockFileSystem({ 'root': { - 'index.js': 'require("C:\\\\root\\\\arbitrary.js");', - 'arbitrary.js': '', + 'index.js': 'require("C:\\\\root\\\\apple.js");', + 'apple.js': '', }, }); @@ -2523,15 +2544,15 @@ describe('DependencyGraph', function() { { id: 'C:\\root\\index.js', path: 'C:\\root\\index.js', - dependencies: ['C:\\root\\arbitrary.js'], + dependencies: ['C:\\root\\apple.js'], isAsset: false, isJSON: false, isPolyfill: false, resolution: undefined, }, { - id: 'C:\\root\\arbitrary.js', - path: 'C:\\root\\arbitrary.js', + id: 'C:\\root\\apple.js', + path: 'C:\\root\\apple.js', dependencies: [], isAsset: false, isJSON: false, @@ -2960,7 +2981,7 @@ describe('DependencyGraph', function() { it('should selectively ignore providesModule in node_modules', function() { var root = '/root'; var otherRoot = '/anotherRoot'; - setMockFileSystem({ + const filesystem = { 'root': { 'index.js': [ '/**', @@ -3051,77 +3072,86 @@ describe('DependencyGraph', function() { 'wazup()', ].join('\n'), }, - }); + }; + setMockFileSystem(filesystem); var dgraph = new DependencyGraph({ ...defaults, roots: [root, otherRoot], }); - return getOrderedDependenciesAsJSON(dgraph, '/root/index.js').then(function(deps) { - expect(deps) - .toEqual([ - { - id: 'index', - path: '/root/index.js', - dependencies: [ - 'shouldWork', - 'dontWork', - 'wontWork', - 'ember', - 'internalVendoredPackage', - 'anotherIndex', - ], - isAsset: false, - isJSON: false, - isPolyfill: false, - resolution: undefined, - }, - { - id: 'shouldWork', - path: '/root/node_modules/react-haste/main.js', - dependencies: ['submodule'], - isAsset: false, - isJSON: false, - isPolyfill: false, - resolution: undefined, - }, - { - id: 'submodule/main.js', - path: '/root/node_modules/react-haste/node_modules/submodule/main.js', - dependencies: [], - isAsset: false, - isJSON: false, - isPolyfill: false, - resolution: undefined, - }, - { - id: 'ember/main.js', - path: '/root/node_modules/ember/main.js', - dependencies: [], - isAsset: false, - isJSON: false, - isPolyfill: false, - resolution: undefined, - }, - { - id: 'internalVendoredPackage', - path: '/root/vendored_modules/a-vendored-package/main.js', - dependencies: [], - isAsset: false, - isJSON: false, - isPolyfill: false, - resolution: undefined, - }, - { - id: 'anotherIndex', - path: '/anotherRoot/index.js', - dependencies: [], - isAsset: false, - isJSON: false, - isPolyfill: false, - resolution: undefined, - }, - ]); + return getOrderedDependenciesAsJSON(dgraph, '/root/index.js').catch( + error => { + expect(error.type).toEqual('UnableToResolveError'); + } + ).then(() => { + filesystem.root['index.js'] = filesystem.root['index.js'] + .replace('require("dontWork")', '') + .replace('require("wontWork")', ''); + dgraph.processFileChange('change', root + '/index.js', mockStat); + return getOrderedDependenciesAsJSON(dgraph, '/root/index.js') + .then(deps => { + expect(deps).toEqual([ + { + id: 'index', + path: '/root/index.js', + dependencies: [ + 'shouldWork', + 'ember', + 'internalVendoredPackage', + 'anotherIndex', + ], + isAsset: false, + isJSON: false, + isPolyfill: false, + resolution: undefined, + }, + { + id: 'shouldWork', + path: '/root/node_modules/react-haste/main.js', + dependencies: ['submodule'], + isAsset: false, + isJSON: false, + isPolyfill: false, + resolution: undefined, + }, + { + id: 'submodule/main.js', + path: '/root/node_modules/react-haste/node_modules/submodule/main.js', + dependencies: [], + isAsset: false, + isJSON: false, + isPolyfill: false, + resolution: undefined, + }, + { + id: 'ember/main.js', + path: '/root/node_modules/ember/main.js', + dependencies: [], + isAsset: false, + isJSON: false, + isPolyfill: false, + resolution: undefined, + }, + { + id: 'internalVendoredPackage', + path: '/root/vendored_modules/a-vendored-package/main.js', + dependencies: [], + isAsset: false, + isJSON: false, + isPolyfill: false, + resolution: undefined, + }, + { + id: 'anotherIndex', + path: '/anotherRoot/index.js', + dependencies: [], + isAsset: false, + isJSON: false, + isPolyfill: false, + resolution: undefined, + }, + ]); + }); }); }); @@ -3180,50 +3210,6 @@ describe('DependencyGraph', function() { }); }); - - it('should ignore modules it cant find (assumes own require system)', function() { - // For example SourceMap.js implements it's own require system. - var root = '/root'; - setMockFileSystem({ - 'root': { - 'index.js': [ - '/**', - ' * @providesModule index', - ' */', - 'require("foo/lol");', - ].join('\n'), - 'node_modules': { - 'foo': { - 'package.json': JSON.stringify({ - name: 'foo', - main: 'main.js', - }), - 'main.js': '/* foo module */', - }, - }, - }, - }); - - var dgraph = new DependencyGraph({ - ...defaults, - roots: [root], - }); - return getOrderedDependenciesAsJSON(dgraph, '/root/index.js').then(function(deps) { - expect(deps) - .toEqual([ - { - id: 'index', - path: '/root/index.js', - dependencies: ['foo/lol'], - isAsset: false, - isJSON: false, - isPolyfill: false, - resolution: undefined, - }, - ]); - }); - }); - it('should work with node packages with a .js in the name', function() { var root = '/root'; setMockFileSystem({ @@ -4522,10 +4508,6 @@ describe('DependencyGraph', function() { }); describe('file watch updating', function() { - var mockStat = { - isDirectory: () => false, - }; - const realPlatform = process.platform; let DependencyGraph; @@ -4549,7 +4531,7 @@ describe('DependencyGraph', function() { 'require("aPackage")', 'require("foo")', ].join('\n'), - 'foo': [ + 'foo.js': [ '/**', ' * @providesModule foo', ' */', @@ -4694,29 +4676,10 @@ describe('DependencyGraph', function() { return getOrderedDependenciesAsJSON(dgraph, '/root/index.js').then(function() { delete filesystem.root.foo; dgraph.processFileChange('delete', root + '/foo.js'); - return getOrderedDependenciesAsJSON(dgraph, '/root/index.js').then(function(deps) { - expect(deps) - .toEqual([ - { - id: 'index', - path: '/root/index.js', - dependencies: ['aPackage', 'foo'], - isAsset: false, - isJSON: false, - isPolyfill: false, - resolution: undefined, - }, - { - id: 'aPackage/main.js', - path: '/root/aPackage/main.js', - dependencies: [], - isAsset: false, - isJSON: false, - isPolyfill: false, - resolution: undefined, - }, - ]); - }); + return getOrderedDependenciesAsJSON(dgraph, '/root/index.js') + .catch(error => { + expect(error.type).toEqual('UnableToResolveError'); + }); }); }); @@ -4831,20 +4794,11 @@ describe('DependencyGraph', function() { assetExts: ['png'], }); - return getOrderedDependenciesAsJSON(dgraph, '/root/index.js').then(function(deps) { - expect(deps) - .toEqual([ - { id: 'index', - path: '/root/index.js', - dependencies: ['./foo.png'], - isAsset: false, - isJSON: false, - isPolyfill: false, - resolution: undefined, - resolveDependency: undefined, - }, - ]); - + return getOrderedDependenciesAsJSON(dgraph, '/root/index.js').catch( + error => { + expect(error.type).toEqual('UnableToResolveError'); + } + ).then(() => { filesystem.root['foo.png'] = ''; dgraph._hasteFS._files[root + '/foo.png'] = ['', 8648460, 1, []]; dgraph.processFileChange('add', root + '/foo.png', mockStat); @@ -4964,25 +4918,41 @@ describe('DependencyGraph', function() { roots: [root], }); return getOrderedDependenciesAsJSON(dgraph, '/root/index.js').then(function() { + filesystem.root['index.js'] = [ + '/**', + ' * @providesModule index', + ' */', + 'require("bPackage")', + ].join('\n'); filesystem.root.aPackage['package.json'] = JSON.stringify({ name: 'bPackage', main: 'main.js', }); + dgraph.processFileChange('change', root + '/index.js', mockStat); dgraph.processFileChange('change', root + '/aPackage/package.json', mockStat); return getOrderedDependenciesAsJSON(dgraph, '/root/index.js').then(function(deps) { expect(deps) .toEqual([ { + dependencies: ['bPackage'], id: 'index', - path: '/root/index.js', - dependencies: ['aPackage'], isAsset: false, isJSON: false, isPolyfill: false, + path: '/root/index.js', resolution: undefined, resolveDependency: undefined, }, + { + dependencies: [], + id: 'aPackage/main.js', + isAsset: false, + isJSON: false, + isPolyfill: false, + path: '/root/aPackage/main.js', + resolution: undefined, + }, ]); }); }); @@ -5319,12 +5289,14 @@ describe('DependencyGraph', function() { }); it('allows setting dependencies for asset modules', () => { - const assetDependencies = ['arbitrary', 'dependencies']; + const assetDependencies = ['/root/apple.png', '/root/banana.png']; setMockFileSystem({ 'root': { 'index.js': 'require("./a.png")', 'a.png' : '', + 'apple.png': '', + 'banana.png': '', }, }); diff --git a/react-packager/src/node-haste/index.js b/react-packager/src/node-haste/index.js index 8f4ccd12..3667077d 100644 --- a/react-packager/src/node-haste/index.js +++ b/react-packager/src/node-haste/index.js @@ -79,7 +79,6 @@ class DependencyGraph { _loading: Promise; constructor({ - // additional arguments for jest-haste-map assetDependencies, assetExts, cache, @@ -297,7 +296,6 @@ class DependencyGraph { platform, platforms: this._opts.platforms, preferNativePlatform: this._opts.preferNativePlatform, - shouldThrowOnUnresolvedErrors: this._opts.shouldThrowOnUnresolvedErrors, }); const response = new ResolutionResponse({transformOptions});