Skip to content

Commit

Permalink
feat(eslint-plugin): Add rule no-reference-import (#625)
Browse files Browse the repository at this point in the history
* feat(eslint-plugin): added no-reference-import rule

* docs: added docs for no-reference-import

* fix(eslint-plugin): collect references when visiting the program node

* feat(eslint-plugin): updated rule to cover more reference directives

* fix(eslint-plugin): deprecated rule and added coverage

* Update README.md
  • Loading branch information
jessetrinity committed Jul 2, 2019
1 parent 34e7d1e commit af70a59
Show file tree
Hide file tree
Showing 9 changed files with 327 additions and 6 deletions.
2 changes: 1 addition & 1 deletion packages/eslint-plugin/README.md
Expand Up @@ -159,7 +159,6 @@ Then you should add `airbnb` (or `airbnb-base`) to your `extends` section of `.e
| [`@typescript-eslint/no-parameter-properties`](./docs/rules/no-parameter-properties.md) | Disallow the use of parameter properties in class constructors | :heavy_check_mark: | | |
| [`@typescript-eslint/no-require-imports`](./docs/rules/no-require-imports.md) | Disallows invocation of `require()` | | | |
| [`@typescript-eslint/no-this-alias`](./docs/rules/no-this-alias.md) | Disallow aliasing `this` | | | |
| [`@typescript-eslint/no-triple-slash-reference`](./docs/rules/no-triple-slash-reference.md) | Disallow `/// <reference path="" />` comments | :heavy_check_mark: | | |
| [`@typescript-eslint/no-type-alias`](./docs/rules/no-type-alias.md) | Disallow the use of type aliases | | | |
| [`@typescript-eslint/no-unnecessary-qualifier`](./docs/rules/no-unnecessary-qualifier.md) | Warns when a namespace qualifier is unnecessary | | :wrench: | :thought_balloon: |
| [`@typescript-eslint/no-unnecessary-type-assertion`](./docs/rules/no-unnecessary-type-assertion.md) | Warns if a type assertion does not change the type of an expression | | :wrench: | :thought_balloon: |
Expand All @@ -179,6 +178,7 @@ Then you should add `airbnb` (or `airbnb-base`) to your `extends` section of `.e
| [`@typescript-eslint/restrict-plus-operands`](./docs/rules/restrict-plus-operands.md) | When adding two variables, operands must both be of type number or of type string | | | :thought_balloon: |
| [`@typescript-eslint/semi`](./docs/rules/semi.md) | Require or disallow semicolons instead of ASI | | :wrench: | |
| [`@typescript-eslint/strict-boolean-expressions`](./docs/rules/strict-boolean-expressions.md) | Restricts the types allowed in boolean expressions | | | :thought_balloon: |
| [`@typescript-eslint/triple-slash-reference`](./docs/rules/triple-slash-reference.md) | Sets preference level for triple slash directives versus ES6-style import declarations | | | |
| [`@typescript-eslint/type-annotation-spacing`](./docs/rules/type-annotation-spacing.md) | Require consistent spacing around type annotations | :heavy_check_mark: | :wrench: | |
| [`@typescript-eslint/unbound-method`](./docs/rules/unbound-method.md) | Enforces unbound methods are called with their expected scope | | | :thought_balloon: |
| [`@typescript-eslint/unified-signatures`](./docs/rules/unified-signatures.md) | Warns for any two overloads that could be unified into one by using a union or an optional/rest parameter | | | |
Expand Down
Expand Up @@ -12,6 +12,8 @@ A triple-slash reference directive is a comment beginning with three slashes fol
ES6 Modules handle this now:
`import animal from "./Animal"`

## DEPRECATED - this rule has been deprecated in favour of [`triple-slash-reference`](./triple-slash-reference.md)

## Rule Details

Does not allow the use of `/// <reference />` comments.
Expand Down
58 changes: 58 additions & 0 deletions packages/eslint-plugin/docs/rules/triple-slash-reference.md
@@ -0,0 +1,58 @@
# Sets preference level for triple slash directives versus ES6-style import declarations. (triple-slash-reference)

Use of triple-slash reference type directives is discouraged in favor of the newer `import` style. This rule allows you to ban use of `/// <reference path="" />`, `/// <reference types="" />`, or `/// <reference lib="" />` directives.

Consider using this rule in place of [`no-triple-slash-reference`](./no-triple-slash-reference.md) which has been deprecated.

## Rule Details

With `{ "path": "never", "types": "never", "lib": "never" }` options set, the following will all be **incorrect** usage:

```ts
/// <reference path="foo" />
/// <reference types="bar" />
/// <reference lib="baz" />
```

Examples of **incorrect** code for the `{ "types": "prefer-import" }` option. Note that these are only errors when **both** stlyes are used for the **same** module:

```ts
/// <reference types="foo" />
import * as foo from 'foo';
```

```ts
/// <reference types="foo" />
import foo = require('foo');
```

With `{ "path": "always", "types": "always", "lib": "always" }` options set, the following will all be **correct** usage:

```ts
/// <reference path="foo" />
/// <reference types="bar" />
/// <reference lib="baz" />
```

Examples of **correct** code for the `{ "types": "prefer-import" }` option:

```ts
import * as foo from 'foo';
```

```ts
import foo = require('foo');
```

## When To Use It

If you want to ban use of one or all of the triple slash reference directives, or any time you might use triple-slash type reference directives and ES6 import declarations in the same file.

## When Not To Use It

If you want to use all flavors of triple slash reference directives.

## Compatibility

- TSLint: [no-reference](http://palantir.github.io/tslint/rules/no-reference/)
- TSLint: [no-reference-import](https://palantir.github.io/tslint/rules/no-reference-import/)
2 changes: 1 addition & 1 deletion packages/eslint-plugin/src/configs/all.json
Expand Up @@ -42,7 +42,6 @@
"@typescript-eslint/no-parameter-properties": "error",
"@typescript-eslint/no-require-imports": "error",
"@typescript-eslint/no-this-alias": "error",
"@typescript-eslint/no-triple-slash-reference": "error",
"@typescript-eslint/no-type-alias": "error",
"@typescript-eslint/no-unnecessary-qualifier": "error",
"@typescript-eslint/no-unnecessary-type-assertion": "error",
Expand All @@ -65,6 +64,7 @@
"@typescript-eslint/restrict-plus-operands": "error",
"semi": "off",
"@typescript-eslint/semi": "error",
"@typescript-eslint/triple-slash-reference": "error",
"@typescript-eslint/type-annotation-spacing": "error",
"@typescript-eslint/unbound-method": "error",
"@typescript-eslint/unified-signatures": "error"
Expand Down
2 changes: 2 additions & 0 deletions packages/eslint-plugin/src/rules/index.ts
Expand Up @@ -54,6 +54,7 @@ import requireArraySortCompare from './require-array-sort-compare';
import restrictPlusOperands from './restrict-plus-operands';
import semi from './semi';
import strictBooleanExpressions from './strict-boolean-expressions';
import tripleSlashReference from './triple-slash-reference';
import typeAnnotationSpacing from './type-annotation-spacing';
import unboundMethod from './unbound-method';
import unifiedSignatures from './unified-signatures';
Expand Down Expand Up @@ -115,6 +116,7 @@ export default {
'restrict-plus-operands': restrictPlusOperands,
semi: semi,
'strict-boolean-expressions': strictBooleanExpressions,
'triple-slash-reference': tripleSlashReference,
'type-annotation-spacing': typeAnnotationSpacing,
'unbound-method': unboundMethod,
'unified-signatures': unifiedSignatures,
Expand Down
6 changes: 4 additions & 2 deletions packages/eslint-plugin/src/rules/no-triple-slash-reference.ts
Expand Up @@ -10,8 +10,10 @@ export default util.createRule({
recommended: 'error',
},
schema: [],
deprecated: true,
replacedBy: ['triple-slash-reference'],
messages: {
tripleSlashReference: 'Do not use a triple slash reference.',
noTripleSlashReference: 'Do not use a triple slash reference.',
},
},
defaultOptions: [],
Expand All @@ -30,7 +32,7 @@ export default util.createRule({
if (referenceRegExp.test(comment.value)) {
context.report({
node: comment,
messageId: 'tripleSlashReference',
messageId: 'noTripleSlashReference',
});
}
});
Expand Down
129 changes: 129 additions & 0 deletions packages/eslint-plugin/src/rules/triple-slash-reference.ts
@@ -0,0 +1,129 @@
import * as util from '../util';
import {
Literal,
Node,
TSExternalModuleReference,
} from '@typescript-eslint/typescript-estree/dist/ts-estree/ts-estree';
import { TSESTree } from '@typescript-eslint/typescript-estree';

type Options = [
{
lib?: 'always' | 'never';
path?: 'always' | 'never';
types?: 'always' | 'never' | 'prefer-import';
}
];
type MessageIds = 'tripleSlashReference';

export default util.createRule<Options, MessageIds>({
name: 'triple-slash-reference',
meta: {
type: 'suggestion',
docs: {
description:
'Sets preference level for triple slash directives versus ES6-style import declarations',
category: 'Best Practices',
recommended: false,
},
messages: {
tripleSlashReference:
'Do not use a triple slash reference for {{module}}, use `import` style instead.',
},
schema: [
{
type: 'object',
properties: {
lib: {
enum: ['always', 'never'],
},
path: {
enum: ['always', 'never'],
},
types: {
enum: ['always', 'never', 'prefer-import'],
},
},
additionalProperties: false,
},
],
},
defaultOptions: [
{
lib: 'always',
path: 'never',
types: 'prefer-import',
},

This comment has been minimized.

Copy link
@mightyiam

mightyiam Aug 22, 2019

Contributor

Could anyone explain why these defaults were chosen and not, for example, never/never/never, please?

This comment has been minimized.

Copy link
@bradzacher

bradzacher Aug 22, 2019

Member

It was just what the rule creator chose and nobody told him otherwise.
If you think it should be different, feel free to raise an issue for discussion

This comment has been minimized.

Copy link
@mightyiam

mightyiam Aug 22, 2019

Contributor

Thank you. I am involved in curating a shareable config. Attempting to discern the right option for it. It's esling-config-standard-with-typescript. I've never used triple slash directives. So I don't have input about this. I thought perhaps these defaults have some wisdom behind them but they might be specific to some circumstances. If anyone would like to give some advice, it's probably best here:
mightyiam/eslint-config-love#134
Thanks for reading.

This comment has been minimized.

Copy link
@bradzacher

bradzacher Aug 22, 2019

Member

I think neverx3 is a good option because in a normal codebase, I think that the uses for triple slash refs is pretty small, so I wouldn't think it's a bad idea to just ban them outright.

],
create(context, [{ lib, path, types }]) {
let programNode: Node;
const sourceCode = context.getSourceCode();
const references: ({
comment: TSESTree.Comment;
importName: string;
})[] = [];

function hasMatchingReference(source: Literal) {
references.forEach(reference => {
if (reference.importName === source.value) {
context.report({
node: reference.comment,
messageId: 'tripleSlashReference',
data: {
module: reference.importName,
},
});
}
});
}
return {
ImportDeclaration(node) {
if (programNode) {
const source = node.source as Literal;
hasMatchingReference(source);
}
},
TSImportEqualsDeclaration(node) {
if (programNode) {
const source = (node.moduleReference as TSExternalModuleReference)
.expression as Literal;
hasMatchingReference(source);
}
},
Program(node) {
if (lib === 'always' && path === 'always' && types == 'always') {
return;
}
programNode = node;
const referenceRegExp = /^\/\s*<reference\s*(types|path|lib)\s*=\s*["|'](.*)["|']/;
const commentsBefore = sourceCode.getCommentsBefore(programNode);

commentsBefore.forEach(comment => {
if (comment.type !== 'Line') {
return;
}
const referenceResult = referenceRegExp.exec(comment.value);

if (referenceResult) {
if (
(referenceResult[1] === 'types' && types === 'never') ||
(referenceResult[1] === 'path' && path === 'never') ||
(referenceResult[1] === 'lib' && lib === 'never')
) {
context.report({
node: comment,
messageId: 'tripleSlashReference',
data: {
module: referenceResult[2],
},
});
return;
}
if (referenceResult[1] === 'types' && types === 'prefer-import') {
references.push({ comment, importName: referenceResult[2] });
}
}
});
},
};
},
});
Expand Up @@ -22,7 +22,7 @@ let a
code: '/// <reference path="Animal" />',
errors: [
{
messageId: 'tripleSlashReference',
messageId: 'noTripleSlashReference',
line: 1,
column: 1,
},
Expand All @@ -36,7 +36,7 @@ let a
parser: '@typescript-eslint/parser',
errors: [
{
messageId: 'tripleSlashReference',
messageId: 'noTripleSlashReference',
line: 2,
column: 1,
},
Expand Down

0 comments on commit af70a59

Please sign in to comment.