From 30fc7389d18e7e5c65f29556fade37188b70f22f Mon Sep 17 00:00:00 2001 From: Amjad Masad Date: Thu, 4 Jun 2015 15:09:13 -0700 Subject: [PATCH] [react-packager] Fix more node_modules resolution rules Summary: @public Fixes #773 This fixes `.json` name resolution. And also reads `package.json` when doing a directory module resolution. The algorithm can be found here: https://nodejs.org/api/modules.html I'll probably start including the node (or browserify) modules test in later diffs to make sure we're fully compliant. Test Plan: * ./runJestTests.sh * ./runJestTests.sh PackagerIntegration * open playground and require a json file * test redbox --- .../__tests__/DependencyGraph-test.js | 69 ++++++++++++++++++- .../haste/DependencyGraph/index.js | 35 ++++------ 2 files changed, 80 insertions(+), 24 deletions(-) diff --git a/packager/react-packager/src/DependencyResolver/haste/DependencyGraph/__tests__/DependencyGraph-test.js b/packager/react-packager/src/DependencyResolver/haste/DependencyGraph/__tests__/DependencyGraph-test.js index 4dce3b8a4..935c4e308 100644 --- a/packager/react-packager/src/DependencyResolver/haste/DependencyGraph/__tests__/DependencyGraph-test.js +++ b/packager/react-packager/src/DependencyResolver/haste/DependencyGraph/__tests__/DependencyGraph-test.js @@ -168,9 +168,11 @@ describe('DependencyGraph', function() { '/**', ' * @providesModule index', ' */', - 'require("./a.json")' + 'require("./a.json")', + 'require("./b")' ].join('\n'), 'a.json': JSON.stringify({}), + 'b.json': JSON.stringify({}), } }); @@ -186,7 +188,7 @@ describe('DependencyGraph', function() { id: 'index', altId: 'package/index', path: '/root/index.js', - dependencies: ['./a.json'], + dependencies: ['./a.json', './b'], isAsset: false, isAsset_DEPRECATED: false, isJSON: undefined, @@ -205,6 +207,17 @@ describe('DependencyGraph', function() { resolution: undefined, resolveDependency: undefined, }, + { + id: 'package/b.json', + isJSON: true, + path: '/root/b.json', + dependencies: [], + isAsset: false, + isAsset_DEPRECATED: false, + isPolyfill: false, + resolution: undefined, + resolveDependency: undefined, + }, ]); }); }); @@ -851,6 +864,58 @@ describe('DependencyGraph', function() { }); }); + pit('should resolve require to main if it is a dir w/ a package.json', function() { + var root = '/root'; + fs.__setMockFilesystem({ + 'root': { + 'package.json': JSON.stringify({ + name: 'test', + }), + 'index.js': 'require("./lib/")', + lib: { + 'package.json': JSON.stringify({ + 'main': 'main.js', + }), + 'index.js': 'lol', + 'main.js': 'lol', + }, + } + }); + + var dgraph = new DependencyGraph({ + roots: [root], + fileWatcher: fileWatcher, + assetExts: ['png', 'jpg'], + }); + return dgraph.getOrderedDependencies('/root/index.js').then(function(deps) { + expect(getDataFromModules(deps)) + .toEqual([ + { + id: 'test/index', + path: '/root/index.js', + dependencies: ['./lib/'], + isAsset: false, + isAsset_DEPRECATED: false, + isJSON: undefined, + isPolyfill: false, + resolution: undefined, + resolveDependency: undefined, + }, + { + id: '/root/lib/main.js', + path: '/root/lib/main.js', + dependencies: [], + isAsset: false, + isAsset_DEPRECATED: false, + isJSON: undefined, + isPolyfill: false, + resolution: undefined, + resolveDependency: undefined, + }, + ]); + }); + }); + pit('should ignore malformed packages', function() { var root = '/root'; fs.__setMockFilesystem({ diff --git a/packager/react-packager/src/DependencyResolver/haste/DependencyGraph/index.js b/packager/react-packager/src/DependencyResolver/haste/DependencyGraph/index.js index 74c131276..1aa8b1290 100644 --- a/packager/react-packager/src/DependencyResolver/haste/DependencyGraph/index.js +++ b/packager/react-packager/src/DependencyResolver/haste/DependencyGraph/index.js @@ -1,6 +1,3 @@ -// TODO -// Fix it to work with tests - /** * Copyright (c) 2015-present, Facebook, Inc. * All rights reserved. @@ -296,16 +293,18 @@ DependecyGraph.prototype.resolveDependency = function( modulePath = path.join(dir, depModuleId); modulePath = browserFieldRedirect(packageJson, modulePath); - // JS modules can be required without extensios. - if (!this._isFileAsset(modulePath) && !modulePath.match(/\.json$/)) { - modulePath = withExtJs(modulePath); - } + dep = this._graph[modulePath] || + this._graph[modulePath + '.js'] || + this._graph[modulePath + '.json']; - dep = this._graph[modulePath]; - - // Maybe the dependency is a directory and there is an index.js inside it. + // Maybe the dependency is a directory and there is a packageJson and/or index.js inside it. if (dep == null) { - dep = this._graph[path.join(dir, depModuleId, 'index.js')]; + var dirPackageJson = this._packageByRoot[path.join(dir, depModuleId).replace(/\/$/, '')]; + if (dirPackageJson) { + dep = this._resolvePackageMain(dirPackageJson); + } else { + dep = this._graph[path.join(dir, depModuleId, 'index.js')]; + } } // Maybe it's an asset with @n.nx resolution and the path doesn't map @@ -444,14 +443,6 @@ DependecyGraph.prototype._processPackage = function(packagePath) { return Promise.resolve(); } - if (packageJson.name == null) { - debug( - 'WARNING: package.json `%s` is missing a name field', - packagePath - ); - return Promise.resolve(); - } - packageJson._root = packageRoot; self._addPackageToIndices(packageJson); @@ -461,14 +452,14 @@ DependecyGraph.prototype._processPackage = function(packagePath) { DependecyGraph.prototype._addPackageToIndices = function(packageJson) { this._packageByRoot[packageJson._root] = packageJson; - if (!this._isInNodeModules(packageJson._root)) { + if (!this._isInNodeModules(packageJson._root) && packageJson.name != null) { this._packagesById[packageJson.name] = packageJson; } }; DependecyGraph.prototype._removePackageFromIndices = function(packageJson) { delete this._packageByRoot[packageJson._root]; - if (!this._isInNodeModules(packageJson._root)) { + if (!this._isInNodeModules(packageJson._root) && packageJson.name != null) { delete this._packagesById[packageJson.name]; } }; @@ -536,7 +527,7 @@ DependecyGraph.prototype._processModule = function(modulePath) { */ DependecyGraph.prototype._lookupName = function(modulePath) { var packageJson = this._lookupPackage(modulePath); - if (packageJson == null) { + if (packageJson == null || packageJson.name == null) { return path.resolve(modulePath); } else { var relativePath =