Use resolve instead of Node.JS require for resolving configuration …

…files (#1172)

* Add `resolve` dependency (and custom typings)

* Use resolve instead of require in resolveConfigurationPaths

Fixes #1171

* Simplify resolveConfigurationPath

`resolve` can handle relative and absolute links.

* path-is-absolute is no longer needed.

* Install test packages in subdirectory, so they aren't in the scope of the main tslint binary

* Run `npm install` in test config directory to install test packages.

* Fix code style issue

* Add dev dependency on npm for grunt run:installTestDeps task

* Revert "Add dev dependency on npm for grunt run:installTestDeps task"

This reverts commit fa0b947.

* Replace run:installTestDeps task with npm-command:test

* Fix lint errors in Gruntfile

* Resolve config file relative to the cwd if it can't be found relative to the parent config

* Make rules in tslint-test-custom-rules package valid rules (copied from noFailRule)

* Add CLI test with a relative extend config

* Add test config package

And install it as a dev dependency

* Add CLI test for extending a package which is installed in tslint.

* Make test packages private

* Fix entry point of test packages

* Simplify non-relative test package

* Add unit test for loadConfigurationFromPath with configs outside of the tslint require path.

Gruntfile.js

@@ -68,6 +68,14 @@ module.exports = function (grunt) {
             test: {
                 tsconfig: "test/tsconfig.json"
             }
+        },
+
+        "npm-command": {
+            test: {
+                options: {
+                    cwd: "./test/config"
+                }
+            }
         }
     });
 
@@ -78,6 +86,7 @@ module.exports = function (grunt) {
     grunt.loadNpmTasks("grunt-run");
     grunt.loadNpmTasks("grunt-tslint");
     grunt.loadNpmTasks("grunt-ts");
+    grunt.loadNpmTasks("grunt-npm-command");
 
     // register custom tasks
     grunt.registerTask("core", [
@@ -87,6 +96,7 @@ module.exports = function (grunt) {
     ]);
     grunt.registerTask("test", [
         "clean:test",
+        "npm-command:test",
         "ts:test",
         "tslint:test",
         "mochaTest",

custom-typings/resolve.d.ts

@@ -0,0 +1,29 @@
+declare module "resolve" {
+  interface ResolveOptions {
+    basedir?: string;
+    extensions?: string[];
+    paths?: string[];
+    moduleDirectory?: string | string[];
+  }
+  interface AsyncResolveOptions extends ResolveOptions {
+    package?: any;
+    readFile?: Function;
+    isFile?: (file: string, cb: Function) => void;
+    packageFilter?: Function;
+    pathFilter?: Function;
+  }
+  interface SyncResolveOptions extends ResolveOptions {
+    readFile?: Function;
+    isFile?: (file: string) => boolean;
+    packageFilter?: Function;
+  }
+  interface ResolveFunction {
+    (id: string, cb: (err: any, res: string, pkg: any) => void): void;
+    (id: string, opts: AsyncResolveOptions, cb: (err: any, res: string, pkg: any) => void): void;
+    sync(id: string, opts?: SyncResolveOptions): string;
+    isCore(pkg: any): any;
+  }
+
+  const resolve: ResolveFunction;
+  export = resolve;
+}

package.json

@@ -25,7 +25,7 @@
     "findup-sync": "~0.3.0",
     "glob": "^6.0.1",
     "optimist": "~0.6.0",
-    "path-is-absolute": "^1.0.0",
+    "resolve": "^1.1.7",
     "underscore.string": "~3.1.1"
   },
   "devDependencies": {
@@ -35,13 +35,13 @@
     "grunt-contrib-clean": "^0.6.0",
     "grunt-jscs": "^1.8.0",
     "grunt-mocha-test": "^0.12.7",
+    "grunt-npm-command": "^0.1.1",
     "grunt-run": "^0.3.0",
     "grunt-ts": "^5.1.0",
     "grunt-tslint": "latest",
     "mocha": "^2.2.5",
     "tslint": "latest",
-    "tslint-test-config": "./test/external/tslint-test-config",
-    "tslint-test-custom-rules": "./test/external/tslint-test-custom-rules",
+    "tslint-test-config-non-relative": "file:test/external/tslint-test-config-non-relative",
     "typescript": "latest"
   },
   "peerDependencies": {

src/configuration.ts

@@ -18,7 +18,7 @@
 import * as fs from "fs";
 import * as path from "path";
 import * as findup from "findup-sync";
-import * as pathIsAbsolute from "path-is-absolute";
+import * as resolve from "resolve";
 
 import {arrayify, objectify, stripComments} from "./utils";
 
@@ -166,22 +166,18 @@ export function loadConfigurationFromPath(configFilePath: string): IConfiguratio
  * @var relativeFilePath Relative path or package name (tslint-config-X) or package short name (X)
  */
 function resolveConfigurationPath(relativeFilePath: string, relativeTo?: string) {
-    let resolvedPath: string;
-    if (pathIsAbsolute(relativeFilePath)) {
-        resolvedPath = relativeFilePath;
-    } else if (relativeFilePath.indexOf(".") === 0) {
-        resolvedPath = getRelativePath(relativeFilePath, relativeTo);
-    } else {
+    const basedir = relativeTo || process.cwd();
+    try {
+        return resolve.sync(relativeFilePath, { basedir });
+    } catch (err) {
         try {
-            resolvedPath = require.resolve(relativeFilePath);
+            return require.resolve(relativeFilePath);
         } catch (err) {
             throw new Error(`Invalid "extends" configuration value - could not require "${relativeFilePath}". ` +
                 "Review the Node lookup algorithm (https://nodejs.org/api/modules.html#modules_all_together) " +
                 "for the approximate method TSLint uses to find the referenced configuration file.");
         }
     }
-
-    return resolvedPath;
 }
 
 export function extendConfigurationFile(config: IConfigurationFile, baseConfig: IConfigurationFile): IConfigurationFile {

src/tsconfig.json

@@ -21,7 +21,7 @@
         "./test/**/*.ts"
     ],
     "files": [
-        "../custom-typings/path-is-absolute.d.ts",
+        "../custom-typings/resolve.d.ts",
         "../typings/colors/colors.d.ts",
         "../typings/diff/diff.d.ts",
         "../typings/findup-sync/findup-sync.d.ts",

test/check-bin.sh

@@ -20,7 +20,7 @@ expectOut () {
   msg=$3
 
   nodeV=`node -v`
-  
+
   # if Node 0.10.*, node will sometimes exit with status 8 when an error is thrown
   if [[ $expect != $actual || $nodeV == v0.10.* && $expect == 1 && $actual == 8 ]] ; then
     echo "$msg: expected $expect got $actual"
@@ -70,6 +70,18 @@ expectOut $? 0 "tslint with with -r pointing to custom rules did not find lint f
 ./bin/tslint -c test/config/tslint-almost-empty.json src/tslint.ts
 expectOut $? 0 "-c relative path without ./ did not work"
 
+# make sure calling tslint with a config file which extends a package relative to the config file works
+./bin/tslint -c test/config/tslint-extends-package-no-mod.json src/tslint.ts
+expectOut $? 0 "tslint (with config file extending relative package) did not work"
+
+# check that a tslint file (outside of the resolution path of the tslint package) can resolve a package that is
+# installed as a dependency of tslint. See palantir/tslint#1172 for details.
+tmpDir=$(mktemp -d)
+echo "{ \"extends\": \"tslint-test-config-non-relative\" }" > $tmpDir/tslint.json
+./bin/tslint -c $tmpDir/tslint.json src/tslint.ts
+expectOut $? 0 "tslint (with config file extending package in tslint scope) did not work"
+rm -r $tmpDir
+
 # make sure tslint --init generates a file
 cd ./bin
 if [ -f tslint.json ]; then

test/config/package.json

@@ -0,0 +1,8 @@
+{
+  "name": "tslint-test-configs",
+  "version": "0.0.1",
+  "dependencies": {
+    "tslint-test-config": "../external/tslint-test-config",
+    "tslint-test-custom-rules": "../external/tslint-test-custom-rules"
+  }
+}

test/configurationTests.ts

@@ -14,6 +14,8 @@
  * limitations under the License.
  */
 
+import * as os from "os";
+import * as path from "path";
 import * as fs from "fs";
 import {IConfigurationFile, extendConfigurationFile, loadConfigurationFromPath} from "../src/configuration";
 
@@ -83,6 +85,35 @@ describe("Configuration", () => {
             });
         });
 
+        describe("with config not relative to tslint", () => {
+            let tmpfile: string;
+            beforeEach(() => {
+                for (let i = 0; i < 5; i++) {
+                    const attempt = path.join(os.tmpdir(), `tslint.test${Math.round(Date.now() * Math.random())}.json`);
+                    if (!fs.existsSync(tmpfile)) {
+                        tmpfile = attempt;
+                        break;
+                    }
+                }
+                if (tmpfile === undefined) {
+                    throw new Error("Couldn't create temp file");
+                }
+            });
+            afterEach(() => {
+                if (tmpfile !== undefined) {
+                    fs.unlinkSync(tmpfile);
+                }
+            });
+            it("extends with package installed relative to tslint", () => {
+                fs.writeFileSync(tmpfile, JSON.stringify({ extends: "tslint-test-config-non-relative" }));
+                let config = loadConfigurationFromPath(tmpfile);
+                assert.deepEqual(config.rules, {
+                    "class-name": true,
+                });
+            });
+        });
+
+
         it("extends with package two levels (and relative path in rulesDirectory)", () => {
             let config = loadConfigurationFromPath("./test/config/tslint-extends-package-two-levels.json");
 

test/external/tslint-test-config-non-relative/package.json

@@ -0,0 +1,8 @@
+{
+  "name": "tslint-test-config-non-relative",
+  "description": "A test package with a tslint config which is installed in the tslint.",
+  "version": "0.0.1",
+  "private": true,
+  "main": "tslint.json",
+  "scripts": {}
+}

test/external/tslint-test-config-non-relative/tslint.json

@@ -0,0 +1,5 @@
+{
+  "rules": {
+    "class-name": true
+  }
+}

test/external/tslint-test-config/package.json

@@ -1,6 +1,7 @@
 {
   "name": "tslint-test-config",
   "version": "0.0.1",
-  "main": "index.js",
+  "private": true,
+  "main": "tslint.json",
   "scripts": {}
 }

test/external/tslint-test-custom-rules/package.json

@@ -1,6 +1,7 @@
 {
   "name": "tslint-test-custom-rules",
   "version": "0.0.1",
+  "private": true,
   "main": "tslint.json",
   "scripts": {}
 }

test/external/tslint-test-custom-rules/rules/ruleOneRule.js

@@ -0,0 +1,27 @@
+var __extends = (this && this.__extends) || function (d, b) {
+    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
+    function __() { this.constructor = d; }
+    d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
+};
+var Lint = require("tslint/lib/lint");
+var Rule = (function (_super) {
+    __extends(Rule, _super);
+    function Rule() {
+        _super.apply(this, arguments);
+    }
+    Rule.prototype.apply = function (sourceFile) {
+        return this.applyWithWalker(new NoFailWalker(sourceFile, this.getOptions()));
+    };
+    return Rule;
+})(Lint.Rules.AbstractRule);
+exports.Rule = Rule;
+var NoFailWalker = (function (_super) {
+    __extends(NoFailWalker, _super);
+    function NoFailWalker() {
+        _super.apply(this, arguments);
+    }
+    NoFailWalker.prototype.visitSourceFile = function (node) {
+        // yay, no failures!
+    };
+    return NoFailWalker;
+})(Lint.RuleWalker);

test/external/tslint-test-custom-rules/rules/ruleThreeRule.js

@@ -0,0 +1,27 @@
+var __extends = (this && this.__extends) || function (d, b) {
+    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
+    function __() { this.constructor = d; }
+    d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
+};
+var Lint = require("tslint/lib/lint");
+var Rule = (function (_super) {
+    __extends(Rule, _super);
+    function Rule() {
+        _super.apply(this, arguments);
+    }
+    Rule.prototype.apply = function (sourceFile) {
+        return this.applyWithWalker(new NoFailWalker(sourceFile, this.getOptions()));
+    };
+    return Rule;
+})(Lint.Rules.AbstractRule);
+exports.Rule = Rule;
+var NoFailWalker = (function (_super) {
+    __extends(NoFailWalker, _super);
+    function NoFailWalker() {
+        _super.apply(this, arguments);
+    }
+    NoFailWalker.prototype.visitSourceFile = function (node) {
+        // yay, no failures!
+    };
+    return NoFailWalker;
+})(Lint.RuleWalker);

test/external/tslint-test-custom-rules/rules/ruleTwoRule.js

@@ -0,0 +1,27 @@
+var __extends = (this && this.__extends) || function (d, b) {
+    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
+    function __() { this.constructor = d; }
+    d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
+};
+var Lint = require("tslint/lib/lint");
+var Rule = (function (_super) {
+    __extends(Rule, _super);
+    function Rule() {
+        _super.apply(this, arguments);
+    }
+    Rule.prototype.apply = function (sourceFile) {
+        return this.applyWithWalker(new NoFailWalker(sourceFile, this.getOptions()));
+    };
+    return Rule;
+})(Lint.Rules.AbstractRule);
+exports.Rule = Rule;
+var NoFailWalker = (function (_super) {
+    __extends(NoFailWalker, _super);
+    function NoFailWalker() {
+        _super.apply(this, arguments);
+    }
+    NoFailWalker.prototype.visitSourceFile = function (node) {
+        // yay, no failures!
+    };
+    return NoFailWalker;
+})(Lint.RuleWalker);

test/tsconfig.json

@@ -20,7 +20,7 @@
         "./rule-tester/*.ts"
     ],
     "files": [
-        "../custom-typings/path-is-absolute.d.ts",
+        "../custom-typings/resolve.d.ts",
         "../typings/colors/colors.d.ts",
         "../typings/diff/diff.d.ts",
         "../typings/findup-sync/findup-sync.d.ts",