From 03cda6b102f431f551a8c5c1807bc6db9f98bde5 Mon Sep 17 00:00:00 2001 From: IllusionMH Date: Wed, 3 Oct 2018 01:00:25 +0300 Subject: [PATCH] Add allow-siblings option to no-relative-imports rule (fixes #493) --- README.md | 2 +- src/noRelativeImportsRule.ts | 44 ++++- src/tests/NoRelativeImportsRuleTests.ts | 222 ++++++++++++++++++------ 3 files changed, 204 insertions(+), 64 deletions(-) diff --git a/README.md b/README.md index 320fd99ed..5e21e4976 100644 --- a/README.md +++ b/README.md @@ -103,7 +103,7 @@ Rule Name | Description | Since `no-multiple-var-decl` | Deprecated - This rule is now part of the base TSLint product as the rule named 'one-variable-per-declaration'. Do not use comma separated variable declarations | 1.0 `no-octal-literal` | Do not use octal literals or escaped octal sequences | 0.0.1 `no-regex-spaces` | Do not use multiple spaces in a regular expression literal. Similar to the [ESLint no-regex-spaces](https://eslint.org/docs/rules/no-regex-spaces.html) rule | 1.0 -`no-relative-imports` | Do not use relative paths when importing external modules or ES6 import declarations. The advantages of removing all relative paths from imports is that 1) the import name will be consistent across all files and subdirectories so searching for usages is much easier. 2) Moving source files to different folders will not require you to edit your import statements. 3) It will be possible to copy and paste import lines between files regardless of the file location. And 4) version control diffs will be simplified by having overall fewer edits to the import lines.| 2.0.5 +`no-relative-imports` | Do not use relative paths when importing external modules or ES6 import declarations. The advantages of removing all relative paths from imports is that 1) the import name will be consistent across all files and subdirectories so searching for usages is much easier. 2) Moving source files to different folders will not require you to edit your import statements. 3) It will be possible to copy and paste import lines between files regardless of the file location. And 4) version control diffs will be simplified by having overall fewer edits to the import lines.

Option `allow-siblings` can be passed to allow relative imports for files in the same or nested folders. | 2.0.5 `no-reserved-keywords` | Do not use reserved keywords as names of local variables, fields, functions, or other identifiers. Since version 2.0.9 this rule accepts a parameter called allow-quoted-properties. If true, interface properties in quotes will be ignored. This can be a useful way to avoid verbose suppress-warning comments for generated d.ts files.
This rule has some overlap with the [tslint variable-name rule](https://palantir.github.io/tslint/rules/variable-name), however, the rule here finds more keywords and more usages.| 0.0.1, 2.0.9 `no-single-line-block-comment` | Avoid single line block comments and use single line comments instead. Block comments do not nest properly and have no advantages over normal single-line comments| 2.0.10 `no-stateless-class` | Deprecated - This rule can be replaced with TSLint's no-unnecessary-class. A stateless class represents a failure in the object oriented design of the system. A class without state is better modeled as a module or given some state. A stateless class is defined as a class with only static members and no parent class.| 2.0.4 diff --git a/src/noRelativeImportsRule.ts b/src/noRelativeImportsRule.ts index d09c74afc..0b6bca45e 100644 --- a/src/noRelativeImportsRule.ts +++ b/src/noRelativeImportsRule.ts @@ -4,8 +4,14 @@ import * as Lint from 'tslint'; import {ErrorTolerantWalker} from './utils/ErrorTolerantWalker'; import {ExtendedMetadata} from './utils/ExtendedMetadata'; +const OPTION_ALLOW_SIBLINGS = 'allow-siblings'; + const FAILURE_STRING_EXT: string = 'External module is being loaded from a relative path. Please use an absolute path: '; const FAILURE_STRING_IMPORT: string = 'Imported module is being loaded from a relative path. Please use an absolute path: '; +const FAILURE_STRING_EXT_SIBLINGS: string = + 'External module path contains reference for parent directory. Please use an absolute path or sibling files/folders: '; +const FAILURE_STRING_IMPORT_SIBLINGS: string = + 'Imported module path contains reference for parent directory. Please use an absolute path or sibling files/folders: '; /** * Implementation of the no-relative-imports rule. @@ -16,9 +22,18 @@ export class Rule extends Lint.Rules.AbstractRule { ruleName: 'no-relative-imports', type: 'maintainability', description: 'Do not use relative paths when importing external modules or ES6 import declarations', - options: null, - optionsDescription: '', - typescriptOnly: true, + options: { + type: 'array', + items: { + type: 'string', + enum: [OPTION_ALLOW_SIBLINGS] + }, + minLength: 0, + maxLength: 1 + }, + optionsDescription: `One argument may be optionally provided: \n\n' + + '* \`${OPTION_ALLOW_SIBLINGS}\` allows relative imports for files in the same or nested folders.`, + typescriptOnly: false, issueClass: 'Ignored', issueType: 'Warning', severity: 'Low', @@ -33,16 +48,26 @@ export class Rule extends Lint.Rules.AbstractRule { } class NoRelativeImportsRuleWalker extends ErrorTolerantWalker { + private allowSiblings: boolean; + + constructor(sourceFile: ts.SourceFile, options: Lint.IOptions) { + super(sourceFile, options); + + this.allowSiblings = options.ruleArguments.indexOf(OPTION_ALLOW_SIBLINGS) > -1; + } + protected visitNode(node: ts.Node): void { if (node.kind === ts.SyntaxKind.ExternalModuleReference) { const moduleExpression: ts.Expression = (node).expression; if (!this.isModuleExpressionValid(moduleExpression)) { - this.addFailureAt(node.getStart(), node.getWidth(), FAILURE_STRING_EXT + node.getText()); + const failureStart = this.allowSiblings ? FAILURE_STRING_EXT_SIBLINGS : FAILURE_STRING_EXT; + this.addFailureAt(node.getStart(), node.getWidth(), failureStart + node.getText()); } } else if (node.kind === ts.SyntaxKind.ImportDeclaration) { const moduleExpression: ts.Expression = (node).moduleSpecifier; if (!this.isModuleExpressionValid(moduleExpression)) { - this.addFailureAt(node.getStart(), node.getWidth(), FAILURE_STRING_IMPORT + node.getText()); + const failureStart = this.allowSiblings ? FAILURE_STRING_IMPORT_SIBLINGS : FAILURE_STRING_IMPORT; + this.addFailureAt(node.getStart(), node.getWidth(), failureStart + node.getText()); } } super.visitNode(node); @@ -51,7 +76,14 @@ class NoRelativeImportsRuleWalker extends ErrorTolerantWalker { private isModuleExpressionValid(expression: ts.Expression): boolean { if (expression.kind === ts.SyntaxKind.StringLiteral) { const moduleName: ts.StringLiteral = expression; - if (moduleName.text[0] === '.') { + + // when no siblings allowed path cannot start with '.' (relative) + if (!this.allowSiblings && moduleName.text[0] === '.') { + return false; + } + + // when siblings allowed path cannot contain '..' (reference to parrent directory) + if (this.allowSiblings && moduleName.text.indexOf('..') > -1) { return false; } } diff --git a/src/tests/NoRelativeImportsRuleTests.ts b/src/tests/NoRelativeImportsRuleTests.ts index b976efb59..bab253dd2 100644 --- a/src/tests/NoRelativeImportsRuleTests.ts +++ b/src/tests/NoRelativeImportsRuleTests.ts @@ -7,69 +7,177 @@ import {TestHelper} from './TestHelper'; describe('noRelativeImportsRule', () : void => { const ruleName : string = 'no-relative-imports'; - it('should pass on absolute path require imports', () : void => { - const script : string = ` - import App = require('App'); - import App = require('common/App'); - `; + describe('no options', () : void => { + it('should pass on absolute path require imports', () : void => { + const script : string = ` + import App = require('App'); + import App = require('common/App'); + `; - TestHelper.assertViolations(ruleName, script, [ ]); - }); + TestHelper.assertViolations(ruleName, script, [ ]); + }); - it('should pass on absolute path ES6 imports', () : void => { - const script : string = ` - import OfficeApp from 'OfficeApp'; - import OfficeApp from 'common/OfficeApp'; - `; + it('should pass on absolute path ES6 imports', () : void => { + const script : string = ` + import OfficeApp from 'OfficeApp'; + import OfficeApp from 'common/OfficeApp'; + `; - TestHelper.assertViolations(ruleName, script, [ ]); - }); + TestHelper.assertViolations(ruleName, script, [ ]); + }); - it('should fail on relative path require import', () : void => { - const script : string = ` - import App = require('./App'); - import App = require('../common/App'); - `; - - TestHelper.assertViolations(ruleName, script, [ - { - failure: 'External module is being loaded from a relative path. Please use an absolute path: require(\'./App\')', - name: 'file.ts', - ruleName: 'no-relative-imports', - startPosition: { character: 26, line: 2 } - }, - { - failure: 'External module is being loaded from a relative path. Please use an absolute path: require(\'../common/App\')', - name: 'file.ts', - ruleName: 'no-relative-imports', - startPosition: { character: 26, line: 3 } - } - ]); - }); + it('should fail on relative path require import', () : void => { + const script : string = ` + import App = require('./App'); + import App = require('./common/App'); + import App = require('../common/App'); + import App = require('./../common/App'); + `; + + TestHelper.assertViolations(ruleName, script, [ + { + failure: 'External module is being loaded from a relative path. ' + + 'Please use an absolute path: require(\'./App\')', + name: 'file.ts', + ruleName: 'no-relative-imports', + startPosition: { character: 30, line: 2 } + }, + { + failure: 'External module is being loaded from a relative path. ' + + 'Please use an absolute path: require(\'./common/App\')', + name: 'file.ts', + ruleName: 'no-relative-imports', + startPosition: { character: 30, line: 3 } + }, + { + failure: 'External module is being loaded from a relative path. ' + + 'Please use an absolute path: require(\'../common/App\')', + name: 'file.ts', + ruleName: 'no-relative-imports', + startPosition: { character: 30, line: 4 } + }, + { + failure: 'External module is being loaded from a relative path. ' + + 'Please use an absolute path: require(\'./../common/App\')', + name: 'file.ts', + ruleName: 'no-relative-imports', + startPosition: { character: 30, line: 5 } + } + ]); + }); - it('should fail on relative path ES6 import', () : void => { - const script : string = ` - import OfficeApp from './OfficeApp'; - import OfficeApp from '../common/OfficeApp'; - `; - - TestHelper.assertViolations(ruleName, script, [ - { - failure: 'Imported module is being loaded from a relative path. ' + - 'Please use an absolute path: import OfficeApp from \'./OfficeApp\';', - name: 'file.ts', - ruleName: 'no-relative-imports', - startPosition: { character: 13, line: 2 } - }, - { - failure: 'Imported module is being loaded from a relative path. ' + - 'Please use an absolute path: import OfficeApp from \'../common/OfficeApp\';', - name: 'file.ts', - ruleName: 'no-relative-imports', - startPosition: { character: 13, line: 3 } - } - ]); + it('should fail on relative path ES6 import', () : void => { + const script : string = ` + import OfficeApp from './OfficeApp'; + import OfficeApp from './common/OfficeApp'; + import OfficeApp from '../common/OfficeApp'; + import OfficeApp from './../common/OfficeApp'; + `; + + TestHelper.assertViolations(ruleName, script, [ + { + failure: 'Imported module is being loaded from a relative path. ' + + 'Please use an absolute path: import OfficeApp from \'./OfficeApp\';', + name: 'file.ts', + ruleName: 'no-relative-imports', + startPosition: { character: 17, line: 2 } + }, + { + failure: 'Imported module is being loaded from a relative path. ' + + 'Please use an absolute path: import OfficeApp from \'./common/OfficeApp\';', + name: 'file.ts', + ruleName: 'no-relative-imports', + startPosition: { character: 17, line: 3 } + }, + { + failure: 'Imported module is being loaded from a relative path. ' + + 'Please use an absolute path: import OfficeApp from \'../common/OfficeApp\';', + name: 'file.ts', + ruleName: 'no-relative-imports', + startPosition: { character: 17, line: 4 } + }, + { + failure: 'Imported module is being loaded from a relative path. ' + + 'Please use an absolute path: import OfficeApp from \'./../common/OfficeApp\';', + name: 'file.ts', + ruleName: 'no-relative-imports', + startPosition: { character: 17, line: 5 } + } + ]); + }); }); + describe('allow-siblings enabled', (): void => { + const options = ['allow-siblings']; + + it('should pass on absolute path require imports when siblings allowed', () : void => { + const script : string = ` + import App = require('App'); + import App = require('common/App'); + `; + TestHelper.assertViolationsWithOptions(ruleName, options, script, [ ]); + }); + + it('should pass on absolute path ES6 imports when siblings allowed', () : void => { + const script : string = ` + import OfficeApp from 'OfficeApp'; + import OfficeApp from 'common/OfficeApp'; + `; + + TestHelper.assertViolationsWithOptions(ruleName, options, script, [ ]); + }); + + it('should fail only on path with \'..\' in require import when siblings allowed', () : void => { + const script : string = ` + import App = require('./App'); + import App = require('./common/App'); + import App = require('../common/App'); + import App = require('./../common/App'); + `; + + TestHelper.assertViolationsWithOptions(ruleName, options, script, [ + { + failure: 'External module path contains reference for parent directory. ' + + 'Please use an absolute path or sibling files/folders: require(\'../common/App\')', + name: 'file.ts', + ruleName: 'no-relative-imports', + startPosition: { character: 30, line: 4 } + }, + { + failure: 'External module path contains reference for parent directory. ' + + 'Please use an absolute path or sibling files/folders: require(\'./../common/App\')', + name: 'file.ts', + ruleName: 'no-relative-imports', + startPosition: { character: 30, line: 5 } + } + ]); + }); + + it('should fail only on path with \'..\' in ES6 import when siblings allowed', () : void => { + const script : string = ` + import OfficeApp from './OfficeApp'; + import OfficeApp from './common/OfficeApp'; + import OfficeApp from '../common/OfficeApp'; + import OfficeApp from './../common/OfficeApp'; + `; + + TestHelper.assertViolationsWithOptions(ruleName, options, script, [ + { + failure: 'Imported module path contains reference for parent directory. ' + + 'Please use an absolute path or sibling files/folders: import OfficeApp from \'../common/OfficeApp\';', + name: 'file.ts', + ruleName: 'no-relative-imports', + startPosition: { character: 17, line: 4 } + }, + { + failure: 'Imported module path contains reference for parent directory. ' + + 'Please use an absolute path or sibling files/folders: import OfficeApp from \'./../common/OfficeApp\';', + name: 'file.ts', + ruleName: 'no-relative-imports', + startPosition: { character: 17, line: 5 } + } + ]); + }); + }); }); /* tslint:enable:no-irregular-whitespace */