fix(ruleLoader): resolve rule files using node path resolution #3108
Conversation
|
Thanks for your interest in palantir/tslint, @devversion! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
There was a problem hiding this comment.
Only found some nits. Looks good otherwise.
This may cause some slowdown when loading rules, but since we cache the result that should not be noticeable.
Re tests: the important thing is that the existing behavior still works. And that's covered by the existing tests.
I don't think we need to add ts-node as dependency just for this one test.
src/ruleLoader.ts
Outdated
| try { | ||
| return require.resolve(path.join(directory, ruleName)); | ||
| } catch (e) { | ||
| return null; |
There was a problem hiding this comment.
Sure. This is probably different from project to project. Going to change it.
src/ruleLoader.ts
Outdated
| if (fs.existsSync(`${fullPath}.js`)) { | ||
| const ruleModule = require(fullPath) as { Rule: RuleConstructor } | undefined; | ||
| const ruleFullPath = getRuleFullPath(directory, ruleName); | ||
| if (ruleFullPath) { |
There was a problem hiding this comment.
Use ruleFullPath !== undefined to make the linter happy
| const fullPath = path.join(directory, ruleName); | ||
| if (fs.existsSync(`${fullPath}.js`)) { | ||
| const ruleModule = require(fullPath) as { Rule: RuleConstructor } | undefined; | ||
| const ruleFullPath = getRuleFullPath(directory, ruleName); |
There was a problem hiding this comment.
No need to rename the variable
There was a problem hiding this comment.
Before it wasn't very explicit. The variable indicated that it would be the path, but it's just a part of the path that misses the extension.
To make it aligned with the method name I thought it would make sense to be more explicit about it.
Custom rule files are no longer being loaded with a hardcoded/fixed extenson (`.js`). With this change all rule files will be resolved using Nodes path resolution. This means that the extension will be detected automatically and other extension loaders like `ts-node` can be used. This means that rules can be also written in TypeScript and loaded through TS-Node.
devversion
force-pushed
the
fix/resolve-rules-using-node
branch
from
8d3378e
to
ac66621
Compare
Custom rule files are no longer being loaded with a hardcoded/fixed extenson (
.js).With this change all rule files will be resolved using Nodes path resolution. This means that the extension will be detected automatically and other extension loaders like
ts-nodecan be used.This means that rules can be also written in TypeScript and loaded through TS-Node.
PR checklist
Overview of change:
Is there anything you'd like reviewers to focus on?
CHANGELOG.md entry:
(Edit by @ajafff: update changelog entry)
[enhancement] custom lint rules will be resolved using node's path resolution to allow for loaders like
ts-node