Cleanup and refactoring of (rule|formatter)Loader (palantir#3412)

src/configuration.ts

@@ -338,7 +338,11 @@ export function extendConfigurationFile(targetConfig: IConfigurationFile,
     };
 }
 
-// returns the absolute path (contrary to what the name implies)
+/**
+ * returns the absolute path (contrary to what the name implies)
+ *
+ * @deprecated use `path.resolve` instead
+ */
 export function getRelativePath(directory?: string | null, relativeTo?: string) {
     if (directory != undefined) {
         const basePath = relativeTo !== undefined ? relativeTo : process.cwd();
@@ -371,15 +375,14 @@ export function getRulesDirectories(directories?: string | string[], relativeTo?
                 }
             }
 
-            const absolutePath = getRelativePath(dir, relativeTo);
+            const absolutePath = relativeTo === undefined ? path.resolve(dir) : path.resolve(relativeTo, dir);
             if (absolutePath !== undefined) {
                 if (!fs.existsSync(absolutePath)) {
                     throw new FatalError(`Could not find custom rule directory: ${dir}`);
                 }
             }
             return absolutePath;
-        })
-        .filter((dir) => dir !== undefined) as string[];
+        });
 }
 
 /**

src/formatterLoader.ts

@@ -17,11 +17,11 @@
 
 import * as fs from "fs";
 import * as path from "path";
+import * as resolve from "resolve";
 import { FormatterConstructor } from "./index";
 import { camelize } from "./utils";
 
-const moduleDirectory = path.dirname(module.filename);
-const CORE_FORMATTERS_DIRECTORY = path.resolve(moduleDirectory, ".", "formatters");
+const CORE_FORMATTERS_DIRECTORY = path.resolve(__dirname, "formatters");
 
 export function findFormatter(name: string | FormatterConstructor, formattersDirectory?: string): FormatterConstructor | undefined {
     if (typeof name === "function") {
@@ -31,7 +31,7 @@ export function findFormatter(name: string | FormatterConstructor, formattersDir
         const camelizedName = camelize(`${name}Formatter`);
 
         // first check for core formatters
-        let Formatter = loadFormatter(CORE_FORMATTERS_DIRECTORY, camelizedName);
+        let Formatter = loadFormatter(CORE_FORMATTERS_DIRECTORY, camelizedName, true);
         if (Formatter !== undefined) {
             return Formatter;
         }
@@ -52,24 +52,37 @@ export function findFormatter(name: string | FormatterConstructor, formattersDir
     }
 }
 
-function loadFormatter(...paths: string[]): FormatterConstructor | undefined {
-    const formatterPath = paths.reduce((p, c) => path.join(p, c), "");
-    const fullPath = path.resolve(moduleDirectory, formatterPath);
-
-    if (fs.existsSync(`${fullPath}.js`)) {
-        const formatterModule = require(fullPath) as { Formatter: FormatterConstructor };
-        return formatterModule.Formatter;
+function loadFormatter(directory: string, name: string, isCore?: boolean): FormatterConstructor | undefined {
+    const formatterPath = path.resolve(path.join(directory, name));
+    let fullPath: string;
+    if (isCore) {
+        fullPath = `${formatterPath}.js`;
+        if (!fs.existsSync(fullPath)) {
+            return undefined;
+        }
+    } else {
+        // Resolve using node's path resolution to allow developers to write custom formatters in TypeScript which can be loaded by TS-Node
+        try {
+            fullPath = require.resolve(formatterPath);
+        } catch {
+            return undefined;
+        }
     }
-
-    return undefined;
+    return (require(fullPath) as { Formatter: FormatterConstructor }).Formatter;
 }
 
 function loadFormatterModule(name: string): FormatterConstructor | undefined {
     let src: string;
     try {
-        src = require.resolve(name);
-    } catch (e) {
-        return undefined;
+        // first try to find a module in the dependencies of the currently linted project
+        src = resolve.sync(name, {basedir: process.cwd()});
+    } catch {
+        try {
+            // if there is no local module, try relative to the installation of TSLint (might be global)
+            src = require.resolve(name);
+        } catch {
+            return undefined;
+        }
     }
     return (require(src) as { Formatter: FormatterConstructor }).Formatter;
 }

src/linter.ts

@@ -24,7 +24,6 @@ import {
     DEFAULT_CONFIG,
     findConfiguration,
     findConfigurationPath,
-    getRelativePath,
     getRulesDirectories,
     IConfigurationFile,
     loadConfigurationFromPath,
@@ -33,7 +32,6 @@ import { removeDisabledFailures } from "./enableDisableRules";
 import { FatalError, isError, showWarningOnce } from "./error";
 import { findFormatter } from "./formatterLoader";
 import { ILinterOptions, LintResult } from "./index";
-import { IFormatter } from "./language/formatter/formatter";
 import { IRule, isTypedRule, Replacement, RuleFailure, RuleSeverity } from "./language/rule/rule";
 import * as utils from "./language/utils";
 import { loadRules } from "./ruleLoader";
@@ -144,16 +142,12 @@ export class Linter {
     }
 
     public getResult(): LintResult {
-        let formatter: IFormatter;
-        const formattersDirectory = getRelativePath(this.options.formattersDirectory);
-
         const formatterName = this.options.formatter !== undefined ? this.options.formatter : "prose";
-        const Formatter = findFormatter(formatterName, formattersDirectory);
-        if (Formatter !== undefined) {
-            formatter = new Formatter();
-        } else {
+        const Formatter = findFormatter(formatterName, this.options.formattersDirectory);
+        if (Formatter === undefined) {
             throw new Error(`formatter '${formatterName}' not found`);
         }
+        const formatter = new Formatter();
 
         const output = formatter.format(this.failures, this.fixes);
 

src/ruleLoader.ts

@@ -18,13 +18,11 @@
 import * as fs from "fs";
 import * as path from "path";
 
-import { getRelativePath } from "./configuration";
 import { FatalError, showWarningOnce } from "./error";
 import { IOptions, IRule, RuleConstructor } from "./language/rule/rule";
 import { arrayify, camelize, dedent, find } from "./utils";
 
-const moduleDirectory = path.dirname(module.filename);
-const CORE_RULES_DIRECTORY = path.resolve(moduleDirectory, ".", "rules");
+const CORE_RULES_DIRECTORY = path.resolve(__dirname, "rules");
 const cachedRules = new Map<string, RuleConstructor | "not-found">();
 
 export function loadRules(ruleOptionsList: IOptions[],
@@ -107,28 +105,14 @@ function transformName(name: string): string {
  * @param ruleName - A name of a rule in filename format. ex) "someLintRule"
  */
 function loadRule(directory: string, ruleName: string): RuleConstructor | "not-found" {
-    const ruleFullPath = getRuleFullPath(directory, ruleName);
-    if (ruleFullPath !== undefined) {
-        const ruleModule = require(ruleFullPath) as { Rule: RuleConstructor } | undefined;
-        if (ruleModule !== undefined) {
-            return ruleModule.Rule;
-        }
-    }
-    return "not-found";
-}
-
-/**
- * Returns the full path to a rule file. Path to rules are resolved using nodes path resolution.
- * This allows developers to write custom rules in TypeScript, which then can be loaded by TS-Node.
- * @param directory - An absolute path to a directory of rules
- * @param ruleName - A name of a rule in filename format. ex) "someLintRule"
- */
-function getRuleFullPath(directory: string, ruleName: string): string | undefined {
+    let ruleFullPath: string;
     try {
-        return require.resolve(path.join(directory, ruleName));
-    } catch (e) {
-        return undefined;
+        // Resolve using node's path resolution to allow developers to write custom rules in TypeScript which can be loaded by TS-Node
+        ruleFullPath = require.resolve(path.join(directory, ruleName));
+    } catch {
+        return "not-found";
     }
+    return (require(ruleFullPath) as { Rule: RuleConstructor }).Rule;
 }
 
 function loadCachedRule(directory: string, ruleName: string, isCustomPath?: boolean): RuleConstructor | undefined {
@@ -140,15 +124,15 @@ function loadCachedRule(directory: string, ruleName: string, isCustomPath?: bool
     }
 
     // get absolute path
-    let absolutePath: string | undefined = directory;
+    let absolutePath: string = directory;
     if (isCustomPath) {
-        absolutePath = getRelativePath(directory);
-        if (absolutePath !== undefined && !fs.existsSync(absolutePath)) {
+        if (!fs.existsSync(directory)) {
             throw new FatalError(`Could not find custom rule directory: ${directory}`);
         }
+        absolutePath = path.resolve(directory);
     }
 
-    const Rule = absolutePath === undefined ? "not-found" : loadRule(absolutePath, ruleName);
+    const Rule = loadRule(absolutePath, ruleName);
 
     cachedRules.set(fullPath, Rule);
     return Rule === "not-found" ? undefined : Rule;

test/config/package.json

@@ -3,6 +3,7 @@
   "version": "0.0.1",
   "dependencies": {
     "tslint-test-config": "../external/tslint-test-config",
-    "tslint-test-custom-rules": "../external/tslint-test-custom-rules"
+    "tslint-test-custom-rules": "../external/tslint-test-custom-rules",
+    "tslint-test-custom-formatter": "../external/tslint-test-custom-formatter"
   }
 }

test/executable/executableTests.ts

@@ -133,6 +133,23 @@ describe("Executable", function(this: Mocha.ISuiteCallbackContext) {
         });
     });
 
+    describe("Custom formatters", () => {
+        it("can be loaded from node_modules", (done) => {
+            execCli(
+                ["-c", "tslint-custom-rules-with-dir.json", "../../src/test.ts", "-t", "tslint-test-custom-formatter"],
+                {
+                    cwd: "./test/config",
+                },
+                (err, stdout) => {
+                    assert.isNotNull(err, "process should exit with error");
+                    assert.strictEqual(err.code, 2, "error code should be 2");
+                    assert.include(stdout, "hello from custom formatter", "stdout should contain output of custom formatter");
+                    done();
+                },
+            );
+        });
+    });
+
     describe("Custom rules", () => {
         it("exits with code 1 if nonexisting custom rules directory is passed", async () => {
             const status = await execRunner(

test/external/tslint-test-custom-formatter/formatter.js

@@ -0,0 +1,24 @@
+"use strict";
+var __extends = (this && this.__extends) || (function () {
+    var extendStatics = Object.setPrototypeOf ||
+        ({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
+        function (d, b) { for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; };
+    return function (d, b) {
+        extendStatics(d, b);
+        function __() { this.constructor = d; }
+        d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
+    };
+})();
+Object.defineProperty(exports, "__esModule", { value: true });
+var lint_1 = require("tslint");
+var Formatter = /** @class */ (function (_super) {
+    __extends(Formatter, _super);
+    function Formatter() {
+        return _super !== null && _super.apply(this, arguments) || this;
+    }
+    Formatter.prototype.format = function () {
+        return "hello from custom formatter";
+    };
+    return Formatter;
+}(lint_1.Formatters.AbstractFormatter));
+exports.Formatter = Formatter;

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

@@ -0,0 +1,7 @@
+{
+  "name": "tslint-test-custom-formatter",
+  "version": "0.0.1",
+  "private": true,
+  "main": "formatter.js",
+  "scripts": {}
+}

test/utils.ts

@@ -29,7 +29,7 @@ export function getSourceFile(fileName: string): ts.SourceFile {
 }
 
 export function getFormatter(formatterName: string): Lint.FormatterConstructor {
-    const formattersDirectory = path.join(path.dirname(module.filename), "../src/formatters");
+    const formattersDirectory = path.join(__dirname, "../src/formatters");
     return Lint.findFormatter(formatterName, formattersDirectory)!;
 }