resolve rulesDirectory - fixes #2163 #2358

donaldpipowitch · 2017-03-16T13:41:31Z

PR checklist

Addresses an existing issue: Feature Request: use node module resolution semantics for rulesDirectory #2163

Overview of change:

When someone uses a rulesDirectory we try to resolve it with Node resolve logic first, before falling back to the old algorithm.

CHANGELOG.md entry:

~[enhancement] rulesDirectory can now be resolved with dir-resolve (#2163)~~
[enhancement] rulesDirectory can now be resolved with Nodes resolve logic, if the directory contains an index.js (#2163)

ajafff · 2017-03-16T14:58:05Z

Would be great if this could work with the main field in package.json. Then you would only need to specify the package name in rulesDirectory and the package.json points to the right subdirectory.

This may conflict with the module resolution that is done for finding configs, though.

donaldpipowitch · 2017-03-16T20:15:06Z

I'm not sure if this would be a good addition. AFAIK main should point to a file, not a directory.

donaldpipowitch · 2017-03-24T12:55:07Z

Who could review this? @nchen63 or @ajafff? Anything I can do to help? :)

ajafff · 2017-03-24T13:32:21Z

Sorry for the delayed response.
Changes LGTM, but my comment from above still stands.

I'm not sure if this would be a good addition. AFAIK main should point to a file, not a directory.

I get your point, but then there is this commit by @jkillian which suggests adding an (empty) index.js to resolve to the correct directory.
buzinas/tslint-eslint-rules@aa17d2e

That's how the configuration directory is resolved.
If rulesDirectory is resolved in the same way, rules and configs would need to be located in the same directory. But that's not too bad, I think.

As a bonus that wouldn't add another dependency, because we can simply use https://www.npmjs.com/package/resolve for both.

ajafff · 2017-03-24T13:40:03Z

Additions to my last comment:

rules and configs would need to be located in the same directory

that's not entirely true. you can still use package/rules although the rules directory also needs an index.js

@donaldpipowitch you should also check the path first before resolving as package. When the path starts with /, ./ or ../ or equals . or .. you don't resolve but use it as a path.

And there need to be tests for this new feature. The best place to start is test/ruleLoaderTests.ts

adidahiya

I agree with @ajafff on most of those points ^

also, yeah, needs tests :)

donaldpipowitch · 2017-03-24T19:28:52Z

Cool. Thank you both for the good feedback. I'll try to make the changes in the next days :)

donaldpipowitch · 2017-04-01T20:48:19Z

I updated my PR.

The logic for resolving the directory is now inside configuration.ts:getRulesDirectories and not ruleLoader.ts:loadCachedRule. It looks like this one is the better place to do this...? getRulesDirectories is called before loadCachedRule.

I get your point, but then there is this commit by @jkillian which suggests adding an (empty) index.js to resolve to the correct directory.
As a bonus that wouldn't add another dependency, because we can simply use https://www.npmjs.com/package/resolve for both.
@donaldpipowitch you should also check the path first before resolving as package. When the path starts with /, ./ or ../ or equals . or .. you don't resolve but use it as a path.

I did all that. I tested this with the "rulesDirectory": [ "tslint-eslint-rules/dist/rules" ], and manually adding an empty index.js into "tslint-eslint-rules/dist/rules". The rule was loaded correctly, but I got a warning when a linter rule failed (Warning: this.createFix is not a function - but it looks like this is a young breaking change #2403 and not fixed by tslint-eslint-rule yet).

And there need to be tests for this new feature.

Can you give me more guidance to test this? Do I need to mock a 3rd party node module to do that correctly?

ajafff · 2017-04-01T21:47:16Z

Do I need to mock a 3rd party node module to do that correctly?

There is already. configurationTests.ts uses two packages for testing, see https://github.com/palantir/tslint/blob/master/test/config/package.json
You can probably use it for your test, too.

ajafff · 2017-04-01T21:53:56Z

src/configuration.ts

@@ -277,6 +277,12 @@ export function getRelativePath(directory?: string | null, relativeTo?: string)
    return undefined;
 }

+// check if directory should be used as path or if it should be resolved like a module
+export function useAsPath(directory: string) {
+    return directory.startsWith("/") || directory.startsWith("./") || directory.startsWith("../")


this should probably include paths starting with node_modules/

to simplify the check, you can use this regex:

/^(?:\.?\.?(?:\/|$)|node_modules\/)/.test(directory)

ajafff · 2017-04-01T21:56:50Z

src/configuration.ts

+                try {
+                    absolutePath = resolve.sync(dir, { basedir: relativeTo }).replace("index.js", "");
+                } catch (err) {
+                    throw new Error(`Could not find custom rule directory: ${dir}`);


instead of throwing here, it should fall back using path as directory

ajafff · 2017-04-01T21:59:16Z

src/configuration.ts

+                }
+            } else {
+                try {
+                    absolutePath = resolve.sync(dir, { basedir: relativeTo }).replace("index.js", "");


just to be safe, because the path may include index.js earlier: .replace(/index.js$/, "")

ajafff · 2017-04-05T14:25:33Z

src/configuration.ts

+    return arrayify(directories)
+        .map((dir) => {
+            let absolutePath;
+            if (useAsPath(dir)) {


to avoid code duplication, consider writing it as follows:

if (!useAsPath(dir)) { try { return resolve.sync(...).replace(...); } catch (err) { // swallow error and fallback to using directory as path } } const absolutePath = getRelativePath(...); if (absolutePath != null) { ... } return absolutePath;

donaldpipowitch · 2017-04-05T19:26:46Z

Is this fine now?

ajafff

Almost done, I had only a few things to comment.

In addition this feature needs a bit of documentation.
https://github.com/palantir/tslint/blob/master/docs/usage/configuration/index.md should contain some information that rulesDirectory tries to resolve to a package in node_modules.
It should also suggest prefixing relative paths with ./ to avoid module resolution.

ajafff · 2017-04-07T09:41:15Z

test/config/tslint-custom-rules-with-package.json

@@ -0,0 +1,3 @@
+{
+  "rulesDirectory": ["tslint-test-custom-rules/rules"]


we should also have tests to ensure that the fallback works. for that you can simply add an entry like "test/external/tslint-test-custom-rules/rules"

btw. the package tslint-test-custom-rules needs a index.js in its rules directory

I tried to add a test, but didn't use "test/external/tslint-test-custom-rules/rules", because it would throw. I'm not sure, this works now as you expected...?

ajafff · 2017-04-07T09:45:36Z

src/configuration.ts

+        .map((dir) => {
+            if (!useAsPath(dir)) {
+                try {
+                    return resolve.sync(dir, { basedir: relativeTo }).replace(/index.js$/, "");


if main in package.json does not point to a file called index.js, this will not quite work. maybe we should use path.dirname(...) instead of .replace(...)

ajafff

Bear with me, I hope that's the last iteration

ajafff · 2017-04-10T13:39:07Z

test/config/tslint-custom-rules-with-package-fallback.json

@@ -0,0 +1,3 @@
+{
+  "rulesDirectory": ["../external/tslint-test-custom-rules/rules"]


Actually I wanted a test where the path was relative but did not start with ./ or ../
Consider adding a folder test/config/relativeRulesDirectory and change this line to "rulesDirectory": ["relativeRulesDirectory"]

ajafff · 2017-04-10T13:42:24Z

test/configurationTests.ts

@@ -252,6 +252,11 @@ describe("Configuration", () => {
            assert.equal("off", config.rules.get("no-eval")!.ruleSeverity);
        });

+        it("resolve rule directory from package", () => {
+            assert.doesNotThrow(() => loadConfigurationFromPath("./test/config/tslint-custom-rules-with-package.json"));
+            assert.doesNotThrow(() => loadConfigurationFromPath("./test/config/tslint-custom-rules-with-package-fallback.json"));


consider checking if it resolved to the expected rulesDirectory in both cases

donaldpipowitch · 2017-04-11T06:20:49Z

@ajafff Thank you for reviewing this :) I updated the tests. I think it looks good now.

donaldpipowitch changed the title ~~fixes #2163~~ resolve rulesDirectory - fixes #2163 Mar 17, 2017

adidahiya suggested changes Mar 24, 2017

View reviewed changes

donaldpipowitch force-pushed the 2163-resolve-rules-directory branch 4 times, most recently from f4c8d0e to c2e1ad7 Compare April 1, 2017 20:38

ajafff reviewed Apr 1, 2017

View reviewed changes

ajafff reviewed Apr 5, 2017

View reviewed changes

ajafff reviewed Apr 7, 2017

View reviewed changes

donaldpipowitch force-pushed the 2163-resolve-rules-directory branch from 988e987 to dd60d03 Compare April 10, 2017 13:00

fixes palantir#2163

c45072f

donaldpipowitch force-pushed the 2163-resolve-rules-directory branch from dd60d03 to c45072f Compare April 10, 2017 13:00

updated docs

aab3b07

ajafff reviewed Apr 10, 2017

View reviewed changes

updated tests

25e34df

ajafff approved these changes Apr 11, 2017

View reviewed changes

adidahiya approved these changes Apr 11, 2017

View reviewed changes

adidahiya mentioned this pull request Apr 11, 2017

Feature Request: use node module resolution semantics for rulesDirectory #2163

Closed

adidahiya merged commit 8978ffa into palantir:master Apr 11, 2017

donaldpipowitch mentioned this pull request Apr 25, 2017

add empty "index.js" to "rules/" directory buzinas/tslint-eslint-rules#214

Open

renovate-bot mentioned this pull request May 3, 2017

Update dependency tslint to version 5.2.0 singapore/lint-condo#282

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resolve rulesDirectory - fixes #2163 #2358

resolve rulesDirectory - fixes #2163 #2358

donaldpipowitch commented Mar 16, 2017 •

edited

ajafff commented Mar 16, 2017

donaldpipowitch commented Mar 16, 2017

donaldpipowitch commented Mar 24, 2017

ajafff commented Mar 24, 2017

ajafff commented Mar 24, 2017 •

edited

adidahiya left a comment

donaldpipowitch commented Mar 24, 2017

donaldpipowitch commented Apr 1, 2017

ajafff commented Apr 1, 2017

ajafff Apr 1, 2017

ajafff Apr 1, 2017

ajafff Apr 1, 2017

ajafff Apr 5, 2017

donaldpipowitch Apr 5, 2017

donaldpipowitch commented Apr 5, 2017

ajafff left a comment

ajafff Apr 7, 2017

ajafff Apr 7, 2017

donaldpipowitch Apr 10, 2017

ajafff Apr 7, 2017

ajafff left a comment

ajafff Apr 10, 2017

ajafff Apr 10, 2017

donaldpipowitch commented Apr 11, 2017

		@@ -0,0 +1,3 @@
		{
		"rulesDirectory": ["tslint-test-custom-rules/rules"]

		@@ -0,0 +1,3 @@
		{
		"rulesDirectory": ["../external/tslint-test-custom-rules/rules"]

resolve rulesDirectory - fixes #2163 #2358

resolve rulesDirectory - fixes #2163 #2358

Conversation

donaldpipowitch commented Mar 16, 2017 • edited

PR checklist

Overview of change:

CHANGELOG.md entry:

ajafff commented Mar 16, 2017

donaldpipowitch commented Mar 16, 2017

donaldpipowitch commented Mar 24, 2017

ajafff commented Mar 24, 2017

ajafff commented Mar 24, 2017 • edited

adidahiya left a comment

Choose a reason for hiding this comment

donaldpipowitch commented Mar 24, 2017

donaldpipowitch commented Apr 1, 2017

ajafff commented Apr 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donaldpipowitch commented Apr 5, 2017

ajafff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajafff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donaldpipowitch commented Apr 11, 2017

donaldpipowitch commented Mar 16, 2017 •

edited

ajafff commented Mar 24, 2017 •

edited