Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Use resolve instead of Node.JS require for resolving configuration files #1172

Merged
merged 21 commits into from
Apr 29, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
19d6068
Add `resolve` dependency (and custom typings)
janslow Apr 26, 2016
ac0f51e
Use resolve instead of require in resolveConfigurationPaths
janslow Apr 26, 2016
49d6b1e
Simplify resolveConfigurationPath
janslow Apr 27, 2016
99110fe
path-is-absolute is no longer needed.
janslow Apr 27, 2016
eb4dde7
Install test packages in subdirectory, so they aren't in the scope of…
janslow Apr 27, 2016
40dece1
Run `npm install` in test config directory to install test packages.
janslow Apr 27, 2016
f74bd30
Fix code style issue
janslow Apr 27, 2016
fa0b947
Add dev dependency on npm for grunt run:installTestDeps task
janslow Apr 27, 2016
fdb7fd8
Revert "Add dev dependency on npm for grunt run:installTestDeps task"
janslow Apr 27, 2016
5663683
Replace run:installTestDeps task with npm-command:test
janslow Apr 27, 2016
5f787b6
Fix lint errors in Gruntfile
janslow Apr 27, 2016
947c0f0
Resolve config file relative to the cwd if it can't be found relative…
janslow Apr 27, 2016
4cb1fdd
Make rules in tslint-test-custom-rules package valid rules (copied fr…
janslow Apr 28, 2016
f9a07d1
Add CLI test with a relative extend config
janslow Apr 28, 2016
b4b2e1a
Add test config package
janslow Apr 28, 2016
a439cac
Add CLI test for extending a package which is installed in tslint.
janslow Apr 28, 2016
b35f877
Make test packages private
janslow Apr 28, 2016
863794a
Fix entry point of test packages
janslow Apr 28, 2016
d4f95de
Simplify non-relative test package
janslow Apr 28, 2016
2496581
Merge branch 'master' into feature/#1171-extends-resolve
janslow Apr 28, 2016
41cf934
Add unit test for loadConfigurationFromPath with configs outside of t…
janslow Apr 28, 2016
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions Gruntfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,14 @@ module.exports = function (grunt) {
test: {
tsconfig: "test/tsconfig.json"
}
},

"npm-command": {
test: {
options: {
cwd: "./test/config"
}
}
}
});

Expand All @@ -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", [
Expand All @@ -87,6 +96,7 @@ module.exports = function (grunt) {
]);
grunt.registerTask("test", [
"clean:test",
"npm-command:test",
"ts:test",
"tslint:test",
"mochaTest",
Expand Down
6 changes: 0 additions & 6 deletions custom-typings/path-is-absolute.d.ts

This file was deleted.

29 changes: 29 additions & 0 deletions custom-typings/resolve.d.ts
Original file line number Diff line number Diff line change
@@ -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;
}
6 changes: 3 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand All @@ -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": {
Expand Down
16 changes: 6 additions & 10 deletions src/configuration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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();
Copy link
Contributor

@jkillian jkillian Apr 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love how you simplified this code!

I'm wondering if we should also attempt to load with regular require.resolve as well. I.e. does it also make sense to look for the node module relative to the installed location of TSLint? I think it does, but only if the "path" specified is not a path but instead a package reference

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would mean it's not the same as the [require.resolve() algorithm](https://nodejs.org/dist/latest-v4.x/docs/api/modules.html#modules_all_together). Before this pull request Y(in the algorithm) was always the location of the tslint package, now it's the location of the parenttslint.json` file (or the tslint package the first time).

It's similar to how you'd expect node -e "require('foo')" to fail if foo was installed globally but not locally.

However, it's arguably a breaking change, so I'm happy to add a fallback which resolves with require.resolve to avoid issues for other users.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with you that ideally extends references in a tslint.json file should only reference packages installed where the file is, but I can see this being a situation where people are relying on other cases, so let's prevent the breaking change here

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 {
Expand Down
2 changes: 1 addition & 1 deletion src/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
14 changes: 13 additions & 1 deletion test/check-bin.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the version of mktemp installed on my Mac, this is unfortunately an invalid usage. I'm okay with getting rid of this test here though, as you have a similar test below. Plus your test below should work on Windows also, which is great.

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
Expand Down
8 changes: 8 additions & 0 deletions test/config/package.json
Original file line number Diff line number Diff line change
@@ -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"
}
}
31 changes: 31 additions & 0 deletions test/configurationTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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");

Expand Down
Empty file.
8 changes: 8 additions & 0 deletions test/external/tslint-test-config-non-relative/package.json
Original file line number Diff line number Diff line change
@@ -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": {}
}
5 changes: 5 additions & 0 deletions test/external/tslint-test-config-non-relative/tslint.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"rules": {
"class-name": true
}
}
3 changes: 2 additions & 1 deletion test/external/tslint-test-config/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{
"name": "tslint-test-config",
"version": "0.0.1",
"main": "index.js",
"private": true,
"main": "tslint.json",
"scripts": {}
}
1 change: 1 addition & 0 deletions test/external/tslint-test-custom-rules/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{
"name": "tslint-test-custom-rules",
"version": "0.0.1",
"private": true,
"main": "tslint.json",
"scripts": {}
}
27 changes: 27 additions & 0 deletions test/external/tslint-test-custom-rules/rules/ruleOneRule.js
Original file line number Diff line number Diff line change
@@ -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);
27 changes: 27 additions & 0 deletions test/external/tslint-test-custom-rules/rules/ruleThreeRule.js
Original file line number Diff line number Diff line change
@@ -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);
27 changes: 27 additions & 0 deletions test/external/tslint-test-custom-rules/rules/ruleTwoRule.js
Original file line number Diff line number Diff line change
@@ -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);
2 changes: 1 addition & 1 deletion test/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down