From e83c140441fb5ce4923d3b574d82e24db772578a Mon Sep 17 00:00:00 2001 From: Jeff Williams Date: Tue, 11 Jul 2017 18:42:23 -0700 Subject: [PATCH] overhaul `path.getResourcePath()` (#1394) The new code is simpler and (hopefully) more correct. It leverages `require.resolve()` to do some of the heavy lifting. --- cli.js | 8 --- lib/jsdoc/path.js | 109 ++++++++++++++++++++------------------- test/specs/jsdoc/path.js | 91 ++++++++++++++++---------------- 3 files changed, 100 insertions(+), 108 deletions(-) diff --git a/cli.js b/cli.js index e71524c8..f0ccf7be 100644 --- a/cli.js +++ b/cli.js @@ -328,14 +328,6 @@ function resolvePluginPaths(paths) { paths.forEach(function(plugin) { var basename = path.basename(plugin); var dirname = path.dirname(plugin); - - // support plugin specification as an installed package, which then may not have a directory - // component, resulting in `path.dirname()` to return `.`, which, if passed on, would result - // in different path resolution semantics - if (dirname.indexOf('.') === 0 && plugin.indexOf('.') !== 0) { - dirname = undefined; - } - var pluginPath = path.getResourcePath(dirname, basename); if (!pluginPath) { diff --git a/lib/jsdoc/path.js b/lib/jsdoc/path.js index ec464795..4f9f6ad0 100644 --- a/lib/jsdoc/path.js +++ b/lib/jsdoc/path.js @@ -85,82 +85,85 @@ exports.commonPrefix = function(paths) { /** * Retrieve the fully qualified path to the requested resource. * - * Plugins and templates will be found somewhat similar to how `require()` works, except that the - * directory in which the JSDoc configuration file is will be considered, too; the JSDoc package's - * directory will be considered as a fallback; and a globally installed resource won't be found - * unless it comes with JSDoc. + * If the resource path is specified as a relative path, JSDoc searches for the resource in the + * following locations, in this order: * - * If the resource path is specified as a path relative to module or package (starting with `.` or - * `..``), JSDoc searches for the path first in the current working directory, then where the JSDoc - * configuration file is located, and finally as a fall-back under the JSDoc directory. Otherwise, - * if the resource path is relative (not starting with a path separator), JSDoc searches first under - * the `node_modules` directory in the current working directory and where the JSDoc configuration - * file is located, and then where JSDoc is installed. + * 1. The current working directory + * 2. The directory where the JSDOc configuration file is located + * 3. The JSDoc directory + * 4. Anyplace where `require()` can find the resource (for example, in your project's + * `node_modules` directory) * - * If the resource path is specified as a fully qualified path, JSDoc uses the path as-is. + * If the resource path is specified as a fully qualified path, JSDoc searches for the resource in + * the following locations, in this order: + * + * 1. The resource path + * 2. Anyplace where `require()` can find the resource (for example, in your project's + * `node_modules` directory) * * @param {string} filepath - The path to the requested resource. May be an absolute path; a path * relative to the JSDoc directory; or a path relative to the current working directory. * @param {string} [filename] - The filename of the requested resource. - * @return {string} The fully qualified path to the requested resource. - * Includes the filename if one was provided. + * @return {string} The fully qualified path to the requested resource. Includes the filename if one + * was provided. */ exports.getResourcePath = function(filepath, filename) { var result = null; - var searchDirs = []; - - function directoryExists(dir) { - var stats; + var searchDirs = [env.pwd, path.dirname(env.opts.configure || ''), env.dirname]; + function exists(p) { try { - stats = fs.statSync(dir); + fs.statSync(p); - return stats.isDirectory(); + return true; } catch (e) { return false; } } - // resources that are installed modules may not have been specified with a filepath - if (!filepath) { - filepath = filename; - filename = undefined; + function resolve(p) { + try { + return require.resolve(p); + } + catch (e) { + return null; + } } - // Special case `node_modules/foo`, to accommodate this legacy workaround advertised by - // third-party plugin and template authors. - if ( /^node_modules\//.test(filepath) ) { - filepath = path.join('.', filepath); - } - - // search in different sets of directories depending on whether filepath is expressly relative - // to "current" directory or not - searchDirs = /^\./.test(filepath) ? - // look first in "current" (where JSDoc was executed), then in directory of config, and - // _only then_ in JSDoc's directory - [env.pwd, path.dirname(env.opts.configure || ''), env.dirname] : - // otherwise, treat as relative to where plugins/templates are found, either as a - // dependency, or under JSDoc itself - [path.join(env.pwd, 'node_modules'), - path.join(path.dirname(env.opts.configure || ''), 'node_modules'), - env.dirname]; - - searchDirs.some(function(dir) { - if (dir) { - dir = path.resolve(dir, filepath); - if ( directoryExists(dir) ) { - result = dir; - - return true; - } + function find(p) { + // does the requested path exist? + if ( exists(p) ) { + result = p; + } + else { + // can `require()` find the requested path? + result = resolve(p); } - return false; - }); + return Boolean(result); + } - if (result) { - result = filename ? path.join(result, filename) : result; + filepath = path.join(filepath, filename || ''); + + // is the filepath absolute? if so, just use it + if ( path.isAbsolute(filepath) ) { + find(filepath); + } + else { + searchDirs.some(function(searchDir) { + if (searchDir) { + return find( path.resolve(path.join(searchDir, filepath)) ); + } + else { + return false; + } + }); + } + + // if we still haven't found the resource, maybe it's an installed module + if (!result) { + result = resolve(filepath); } return result; diff --git a/test/specs/jsdoc/path.js b/test/specs/jsdoc/path.js index f79bf5d4..533384ba 100644 --- a/test/specs/jsdoc/path.js +++ b/test/specs/jsdoc/path.js @@ -132,28 +132,26 @@ describe('jsdoc/path', function() { }); describe('getResourcePath', function() { + var oldConf; var oldPwd; beforeEach(function() { + oldConf = env.opts.configure; oldPwd = env.pwd; + + env.opts.configure = path.join(env.dirname, 'lib', 'conf.json'); env.pwd = __dirname; }); afterEach(function() { + env.opts.configure = oldConf; env.pwd = oldPwd; }); - it('resolves package-relative path that exists', function() { - var resolved = path.getResourcePath('plugins'); + it('resolves pwd-relative path that exists', function() { + var resolved = path.getResourcePath('doclet'); - expect(resolved).not.toBeNull(); - expect( path.isAbsolute(resolved) ).toBe(true); - }); - - it('fails to resolve package-relative path that exists in ./', function() { - var resolved = path.getResourcePath('util'); - - expect(resolved).toBeNull(); + expect(resolved).toBe( path.join(__dirname, 'doclet.js') ); }); it('resolves relative to ./ path that exists', function() { @@ -161,59 +159,58 @@ describe('jsdoc/path', function() { var p = ['.', 'util'].join(path.sep); var resolved = path.getResourcePath(p); - expect(resolved).not.toBeNull(); - expect( path.isAbsolute(resolved) ).toBe(true); + expect(resolved).toBe( path.join(__dirname, 'util') ); }); it('resolves relative to ../ path that exists', function() { var p = path.join('..', 'jsdoc', 'util'); var resolved = path.getResourcePath(p); - expect(resolved).not.toBeNull(); - expect( path.isAbsolute(resolved) ).toBe(true); - }); - - it('resolves relative to ../ path that exists in ../ and package', function() { - var prel = path.join('..', 'plugins'); - var pabs = 'plugins'; - var resolved = path.getResourcePath(prel); - - expect(resolved).not.toBeNull(); - expect( path.getResourcePath(pabs) ).not.toBeNull(); - expect( path.isAbsolute(resolved) ).toBe(true); - expect( path.getResourcePath(pabs) ).not.toBe(resolved); - }); - - it('resolves relative to . path that exists in package', function() { - var p = path.join('.', 'plugins'); - var resolved = path.getResourcePath(p); - - expect(resolved).not.toBeNull(); - expect( path.isAbsolute(resolved) ).toBe(true); + expect(resolved).toBe( path.join(__dirname, 'util') ); }); it('resolves path using node_modules/', function() { - var p = path.join('node_modules', 'marked'); - var resolved = path.getResourcePath( path.dirname(p), path.basename(p) ); + var resolved = path.getResourcePath('node_modules', 'catharsis'); - expect(resolved).not.toBeNull(); - expect( path.isAbsolute(resolved) ).toBe(true); + expect(resolved).toBe( path.join(env.dirname, 'node_modules', 'catharsis') ); }); - it('resolves installed module using \'module\'', function() { - var p = 'marked'; - var resolved = path.getResourcePath(undefined, p); + it('resolves paths relative to the configuration file\'s path', function() { + var resolved = path.getResourcePath('jsdoc'); - expect(resolved).not.toBeNull(); - expect( path.isAbsolute(resolved) ).toBe(true); + expect(resolved).toBe( path.join(env.dirname, 'lib', 'jsdoc') ); }); - it('leaves an absolute path as is', function() { - var p = path.resolve( path.join(env.dirname, 'anything') ); - var resolved = path.getResourcePath(path.dirname(p), 'anything'); + it('resolves paths relative to the JSDoc path', function() { + var resolved = path.getResourcePath( path.join('lib', 'jsdoc') ); - expect(resolved).not.toBeNull(); - expect(p).toBe(resolved); + expect(resolved).toBe( path.join(env.dirname, 'lib', 'jsdoc') ); + }); + + it('resolves installed module', function() { + var resolved = path.getResourcePath('catharsis'); + + expect(resolved).toBe( path.join(env.dirname, 'node_modules', 'catharsis', + 'catharsis.js') ); + }); + + it('fails to find a relative path that does not exist', function() { + var resolved = path.getResourcePath('foo'); + + expect(resolved).toBeNull(); + }); + + it('finds an absolute path that does exist', function() { + var p = path.join(env.dirname, 'lib'); + var resolved = path.getResourcePath(p); + + expect(resolved).toBe(p); + }); + + it('fails to find an absolute path that does not exist', function() { + var resolved = path.getResourcePath( path.join(env.dirname, 'foo') ); + + expect(resolved).toBeNull(); }); }); });