From e2aa98d75cbbf49c8f8f5fe645da4da958efc6c9 Mon Sep 17 00:00:00 2001 From: Taeheon Kim Date: Wed, 17 Oct 2018 14:37:52 +0900 Subject: [PATCH 01/13] Make more arguments can be passed to options by using index flag and make more state for the later --- src/importNameRule.ts | 53 +++++++++++++++++++++++++++++++------------ 1 file changed, 39 insertions(+), 14 deletions(-) diff --git a/src/importNameRule.ts b/src/importNameRule.ts index 089db7b43..f9dc90d11 100644 --- a/src/importNameRule.ts +++ b/src/importNameRule.ts @@ -31,25 +31,50 @@ export class Rule extends Lint.Rules.AbstractRule { } } +type Replacement = { [index: string]: string; }; +type IgnoredList = string[]; +type ConfigKey = 'ignoreExternalModule'; +type Config = { [index in ConfigKey]: boolean; }; + +type Option = { + replacements: Replacement + ignoredList: IgnoredList + config: Config +}; + class ImportNameRuleWalker extends ErrorTolerantWalker { - private replacements: { [index: string]: string; }; + private option: Option; constructor(sourceFile: ts.SourceFile, options: Lint.IOptions) { super(sourceFile, options); - this.replacements = this.extractOptions(); + this.option = this.extractOptions(); + } + + private extractOptions(): Option { + const result : Option = { + replacements: {}, + ignoredList: [], + config: { + ignoreExternalModule: true + } + }; + + this.getOptions().forEach((opt: any, index: number) => { + if (index === 1 && typeof(opt) === 'object') { + result.replacements = this.extractReplacements(opt); + } + }); + + return result; } - private extractOptions(): { [index: string]: string; } { - const result : { [index: string]: string; } = {}; - this.getOptions().forEach((opt: any) => { - if (typeof(opt) === 'object') { - Object.keys(opt).forEach((key: string): void => { - const value: any = opt[key]; - if (typeof value === 'string') { - result[key] = value; - } - }); + private extractReplacements(opt: Replacement): Replacement { + const result: Replacement = {}; + Object.keys(opt).forEach((key: string): void => { + const value: any = opt[key]; + if (typeof value === 'string') { + result[key] = value; } }); return result; @@ -115,10 +140,10 @@ class ImportNameRuleWalker extends ErrorTolerantWalker { // `import cgi from 'fs-util/cgi-common'` // The Valid specifiers are: [cgiCommon, fs-util/cgi-common, cgi-common] const allowedReplacementKeys: string[] = [expectedImportedName, moduleName, moduleName.replace(/.*\//, '')]; - return Utils.exists(Object.keys(this.replacements), (replacementKey: string): boolean => { + return Utils.exists(Object.keys(this.option.replacements), (replacementKey: string): boolean => { for (let index = 0; allowedReplacementKeys.length > index; index = index + 1) { if (replacementKey === allowedReplacementKeys[index]) { - return importedName === this.replacements[replacementKey]; + return importedName === this.option.replacements[replacementKey]; } } return false; From 0ff5d1fad61366d0046c90eab74881cf15802627 Mon Sep 17 00:00:00 2001 From: Taeheon Kim Date: Wed, 17 Oct 2018 14:44:58 +0900 Subject: [PATCH 02/13] Extract checking replacements logic to function for more checking for the later --- src/importNameRule.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/importNameRule.ts b/src/importNameRule.ts index f9dc90d11..f63705585 100644 --- a/src/importNameRule.ts +++ b/src/importNameRule.ts @@ -134,6 +134,10 @@ class ImportNameRuleWalker extends ErrorTolerantWalker { return true; } + return this.checkReplacementsExist(importedName, expectedImportedName, moduleName); + } + + private checkReplacementsExist(importedName: string, expectedImportedName: string, moduleName: string) { // Allowed Replacement keys are specifiers that are allowed when overriding or adding exceptions // to import-name rule. // Example: for below import statement From 1242f10928bf576fcf03d34b87388515b4034fd6 Mon Sep 17 00:00:00 2001 From: Taeheon Kim Date: Wed, 17 Oct 2018 15:05:39 +0900 Subject: [PATCH 03/13] Extract ignoredList from third argument --- src/importNameRule.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/importNameRule.ts b/src/importNameRule.ts index f63705585..772d1c652 100644 --- a/src/importNameRule.ts +++ b/src/importNameRule.ts @@ -64,6 +64,10 @@ class ImportNameRuleWalker extends ErrorTolerantWalker { if (index === 1 && typeof(opt) === 'object') { result.replacements = this.extractReplacements(opt); } + + if (index === 2 && Array.isArray(opt)) { + result.ignoredList = this.extractIgnoredList(opt); + } }); return result; @@ -80,6 +84,10 @@ class ImportNameRuleWalker extends ErrorTolerantWalker { return result; } + private extractIgnoredList(opt: IgnoredList): IgnoredList { + return opt.filter((moduleName: string) => typeof moduleName === 'string'); + } + protected visitImportEqualsDeclaration(node: ts.ImportEqualsDeclaration): void { const name: string = node.name.text; From f3234c45ec734b54d2c7ace8b8225afcd68a8e9a Mon Sep 17 00:00:00 2001 From: Taeheon Kim Date: Wed, 17 Oct 2018 15:37:22 +0900 Subject: [PATCH 04/13] Make "isImportNameValid()" return true when ignoredList has moduleName that is currently in a validation process Add "es7" to "lib" at tsconfig.json for using Array.includes() with string[] type --- src/importNameRule.ts | 23 +++++++++++++++++++---- tsconfig.json | 1 + 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/importNameRule.ts b/src/importNameRule.ts index 772d1c652..f248fbdf4 100644 --- a/src/importNameRule.ts +++ b/src/importNameRule.ts @@ -142,23 +142,38 @@ class ImportNameRuleWalker extends ErrorTolerantWalker { return true; } - return this.checkReplacementsExist(importedName, expectedImportedName, moduleName); + const isReplacementsExist = this.checkReplacementsExist(importedName, expectedImportedName, moduleName, this.option.replacements); + if (isReplacementsExist) { + return true; + } + + const isIgnoredModuleExist = this.checkIgnoredListExists(moduleName, this.option.ignoredList); + if (isIgnoredModuleExist) { + return true; + } + + return false; } - private checkReplacementsExist(importedName: string, expectedImportedName: string, moduleName: string) { + private checkReplacementsExist(importedName: string, expectedImportedName: string, moduleName: string, replacements: Replacement) + : boolean { // Allowed Replacement keys are specifiers that are allowed when overriding or adding exceptions // to import-name rule. // Example: for below import statement // `import cgi from 'fs-util/cgi-common'` // The Valid specifiers are: [cgiCommon, fs-util/cgi-common, cgi-common] const allowedReplacementKeys: string[] = [expectedImportedName, moduleName, moduleName.replace(/.*\//, '')]; - return Utils.exists(Object.keys(this.option.replacements), (replacementKey: string): boolean => { + return Utils.exists(Object.keys(replacements), (replacementKey: string): boolean => { for (let index = 0; allowedReplacementKeys.length > index; index = index + 1) { if (replacementKey === allowedReplacementKeys[index]) { - return importedName === this.option.replacements[replacementKey]; + return importedName === replacements[replacementKey]; } } return false; }); } + + private checkIgnoredListExists(moduleName: string, ignoredList: IgnoredList): boolean { + return ignoredList.includes(moduleName); + } } diff --git a/tsconfig.json b/tsconfig.json index dcbac87e4..25ceea4d4 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -15,6 +15,7 @@ "jsx": "react", "lib": [ "es6", + "es7", "dom", "scripthost" ], From 169d2af47ffbc4b81b8fe0a7473f601459f2211f Mon Sep 17 00:00:00 2001 From: Taeheon Kim Date: Wed, 17 Oct 2018 16:23:20 +0900 Subject: [PATCH 05/13] Extra config from fourth argument --- src/importNameRule.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/importNameRule.ts b/src/importNameRule.ts index f248fbdf4..156367ac9 100644 --- a/src/importNameRule.ts +++ b/src/importNameRule.ts @@ -68,6 +68,10 @@ class ImportNameRuleWalker extends ErrorTolerantWalker { if (index === 2 && Array.isArray(opt)) { result.ignoredList = this.extractIgnoredList(opt); } + + if (index === 3 && typeof(opt) === 'object') { + result.config = this.extractConfig(opt); + } }); return result; @@ -88,6 +92,17 @@ class ImportNameRuleWalker extends ErrorTolerantWalker { return opt.filter((moduleName: string) => typeof moduleName === 'string'); } + private extractConfig(opt: Config): Config { + const configKeyLlist: ConfigKey[] = ['ignoreExternalModule']; + return Object.keys(opt).reduce((accum: Config, key: string) => { + if (configKeyLlist.includes(key)) { + accum[key] = opt[key]; + return accum; + } + return accum; + }, { ignoreExternalModule: true }); + } + protected visitImportEqualsDeclaration(node: ts.ImportEqualsDeclaration): void { const name: string = node.name.text; From 702064c259b952b94bfb754c00063360a764de08 Mon Sep 17 00:00:00 2001 From: Taeheon Kim Date: Wed, 17 Oct 2018 16:48:30 +0900 Subject: [PATCH 06/13] Ignore if isExternalLibraryImport flag is true from runtime node object --- src/importNameRule.ts | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/src/importNameRule.ts b/src/importNameRule.ts index 156367ac9..c5c7a7d94 100644 --- a/src/importNameRule.ts +++ b/src/importNameRule.ts @@ -134,7 +134,7 @@ class ImportNameRuleWalker extends ErrorTolerantWalker { private validateImport(node: ts.ImportEqualsDeclaration | ts.ImportDeclaration, importedName: string, moduleName: string): void { let expectedImportedName = moduleName.replace(/.*\//, ''); // chop off the path expectedImportedName = this.makeCamelCase(expectedImportedName); - if (this.isImportNameValid(importedName, expectedImportedName, moduleName) === false) { + if (this.isImportNameValid(importedName, expectedImportedName, moduleName, node) === false) { const message: string = `Misnamed import. Import should be named '${expectedImportedName}' but found '${importedName}'`; const nameNode = node.kind === ts.SyntaxKind.ImportEqualsDeclaration ? (node).name @@ -152,7 +152,7 @@ class ImportNameRuleWalker extends ErrorTolerantWalker { }); } - private isImportNameValid(importedName: string, expectedImportedName: string, moduleName: string): boolean { + private isImportNameValid(importedName: string, expectedImportedName: string, moduleName: string, node: any): boolean { if (expectedImportedName === importedName) { return true; } @@ -167,6 +167,11 @@ class ImportNameRuleWalker extends ErrorTolerantWalker { return true; } + const ignoreThisExternalModule = this.checkIgnoreExternalModule(moduleName, node, this.option.config); + if (ignoreThisExternalModule) { + return true; + } + return false; } @@ -191,4 +196,19 @@ class ImportNameRuleWalker extends ErrorTolerantWalker { private checkIgnoredListExists(moduleName: string, ignoredList: IgnoredList): boolean { return ignoredList.includes(moduleName); } + + private checkIgnoreExternalModule(moduleName: string, node: any, opt: Config): boolean { + if (opt.ignoreExternalModule && node.parent !== undefined && node.parent.resolvedModules !== undefined) { + let ignoreThisExternalModule = false; + for (const [key, value] of node.parent.resolvedModules) { + if (key === moduleName && value.isExternalLibraryImport === true) { + ignoreThisExternalModule = true; + break; + } + } + return ignoreThisExternalModule; + } + + return false; + } } From 1c2f2998a3a208ae88b9d98ebcc0615c64e2f7d5 Mon Sep 17 00:00:00 2001 From: Taeheon Kim Date: Wed, 17 Oct 2018 17:34:13 +0900 Subject: [PATCH 07/13] Add Tests that can be tested with "grunt all" - Need Help I want to write a test about ignoring node_moduels feature, but "node" object is only given at runtime. I don't know how to write a test of it. --- src/tests/ImportNameRuleTests.ts | 57 +++++++++++++++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/src/tests/ImportNameRuleTests.ts b/src/tests/ImportNameRuleTests.ts index 793861d2f..8e03cb817 100644 --- a/src/tests/ImportNameRuleTests.ts +++ b/src/tests/ImportNameRuleTests.ts @@ -167,5 +167,60 @@ describe('importNameRule', () : void => { 'myModule': 'pqr' }]; TestHelper.assertViolationsWithOptions(ruleName, options, script, []); - }) + }); + + it('should pass on ignoring modules from ignoredList(string[] from third argument)', () : void => { + const script : string = ` + import pkg from 'fs/package-name', + import abc from 'abc-tag', + import pqr from 'my-module' + import what from 'what-module' + import Up from 'up-module' + `; + const options: any[] = [ true, { + 'fs/package-name': 'pkg', + 'abc-tag': 'abc', + 'myModule': 'pqr' + }, ['what-module', 'up-module']]; + TestHelper.assertViolationsWithOptions(ruleName, options, script, []); + }); + + it('should pass on ignoring third argument value other than string[]', () : void => { + const script : string = ` + import pkg from 'fs/package-name', + import abc from 'abc-tag', + import pqr from 'my-module' + import what from 'what-module' + import Up from 'up-module' + `; + const options: any[] = [ true, { + 'fs/package-name': 'pkg', + 'abc-tag': 'abc', + 'myModule': 'pqr' + }, [123, { 'whatever': 'object' }]]; + TestHelper.assertViolationsWithOptions(ruleName, options, script, [ + { + "failure": "Misnamed import. Import should be named 'whatModule' but found 'what'", + "name": Utils.absolutePath("file.ts"), + "ruleName": "import-name", + "startPosition": { "character": 9, "line": 5 }, + "fix": { + "innerStart": 130, + "innerLength": 4, + "innerText": "whatModule" + } + }, + { + "failure": "Misnamed import. Import should be named 'upModule' but found 'Up'", + "name": Utils.absolutePath("file.ts"), + "ruleName": "import-name", + "startPosition": { "character": 9, "line": 6 }, + "fix": { + "innerStart": 169, + "innerLength": 2, + "innerText": "upModule" + } + } + ]); + }); }); From 3279457a4f06cb52d1fbdf6f620a43c4c7e11d8d Mon Sep 17 00:00:00 2001 From: Taeheon Kim Date: Wed, 17 Oct 2018 21:05:27 +0900 Subject: [PATCH 08/13] for.. of doesn't support Map at es5 target. It just changes to for loop and silently doesn't go into for loop. I changed it to forEach. Now It's working. --- src/importNameRule.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/importNameRule.ts b/src/importNameRule.ts index c5c7a7d94..8655b2ea4 100644 --- a/src/importNameRule.ts +++ b/src/importNameRule.ts @@ -200,15 +200,13 @@ class ImportNameRuleWalker extends ErrorTolerantWalker { private checkIgnoreExternalModule(moduleName: string, node: any, opt: Config): boolean { if (opt.ignoreExternalModule && node.parent !== undefined && node.parent.resolvedModules !== undefined) { let ignoreThisExternalModule = false; - for (const [key, value] of node.parent.resolvedModules) { + node.parent.resolvedModules.forEach((value: any, key: string) => { if (key === moduleName && value.isExternalLibraryImport === true) { ignoreThisExternalModule = true; - break; } - } + }); return ignoreThisExternalModule; } - return false; } } From 8a0282e2b55536892c517a28095b799995257670 Mon Sep 17 00:00:00 2001 From: Taeheon Kim Date: Tue, 23 Oct 2018 20:13:15 +0900 Subject: [PATCH 09/13] 1. Remove "es7" from tsconfig 2. Without es7 support, Array.includes doesnt work. I changed it using filter function. (This is the reason why I added es7 support) I think Array.includes() is not that 'absolutely needed' I just removed it. --- src/importNameRule.ts | 4 ++-- tsconfig.json | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/importNameRule.ts b/src/importNameRule.ts index 8655b2ea4..c44d5a6b9 100644 --- a/src/importNameRule.ts +++ b/src/importNameRule.ts @@ -95,7 +95,7 @@ class ImportNameRuleWalker extends ErrorTolerantWalker { private extractConfig(opt: Config): Config { const configKeyLlist: ConfigKey[] = ['ignoreExternalModule']; return Object.keys(opt).reduce((accum: Config, key: string) => { - if (configKeyLlist.includes(key)) { + if (configKeyLlist.filter((configKey: string) => configKey === key).length >= 1) { accum[key] = opt[key]; return accum; } @@ -194,7 +194,7 @@ class ImportNameRuleWalker extends ErrorTolerantWalker { } private checkIgnoredListExists(moduleName: string, ignoredList: IgnoredList): boolean { - return ignoredList.includes(moduleName); + return ignoredList.filter((ignoredModule: string) => ignoredModule === moduleName).length >= 1; } private checkIgnoreExternalModule(moduleName: string, node: any, opt: Config): boolean { diff --git a/tsconfig.json b/tsconfig.json index 25ceea4d4..5b9d58327 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -15,7 +15,6 @@ "jsx": "react", "lib": [ "es6", - "es7", "dom", "scripthost" ], @@ -29,4 +28,4 @@ "include": [ "src" ] -} +} \ No newline at end of file From d4281c0e0175876d081a31eb7d7ab3ccd8c1e6b4 Mon Sep 17 00:00:00 2001 From: Taeheon Kim Date: Tue, 23 Oct 2018 20:28:06 +0900 Subject: [PATCH 10/13] comment added? --- src/importNameRule.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/importNameRule.ts b/src/importNameRule.ts index c44d5a6b9..b51ce53b3 100644 --- a/src/importNameRule.ts +++ b/src/importNameRule.ts @@ -193,10 +193,12 @@ class ImportNameRuleWalker extends ErrorTolerantWalker { }); } + // Ignore array of strings that comes from third argument. private checkIgnoredListExists(moduleName: string, ignoredList: IgnoredList): boolean { return ignoredList.filter((ignoredModule: string) => ignoredModule === moduleName).length >= 1; } + // Ignore NPM installed modules by checking its module path at runtime private checkIgnoreExternalModule(moduleName: string, node: any, opt: Config): boolean { if (opt.ignoreExternalModule && node.parent !== undefined && node.parent.resolvedModules !== undefined) { let ignoreThisExternalModule = false; From d5b78f979070f4231c7fe52191123848cc2cf87e Mon Sep 17 00:00:00 2001 From: Taeheon Kim Date: Wed, 24 Oct 2018 02:29:29 +0900 Subject: [PATCH 11/13] comment for interfaces --- src/importNameRule.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/importNameRule.ts b/src/importNameRule.ts index 52eb4fe5d..242ae716b 100644 --- a/src/importNameRule.ts +++ b/src/importNameRule.ts @@ -37,7 +37,8 @@ type IgnoredList = string[]; type ConfigKey = 'ignoreExternalModule'; type Config = { [index in ConfigKey]: unknown; }; -// +// This is for temporarily reolving type errors. Actual runtime Node, SourceFile object +// has those properties. interface RuntimeSourceFile extends ts.SourceFile { resolvedModules: Map; } From 9a4e7089374006828956c7fc3ed6b047758dc929 Mon Sep 17 00:00:00 2001 From: Taeheon Kim Date: Wed, 24 Oct 2018 02:37:27 +0900 Subject: [PATCH 12/13] Add optionExamples --- src/importNameRule.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/importNameRule.ts b/src/importNameRule.ts index d2367550f..f2050993d 100644 --- a/src/importNameRule.ts +++ b/src/importNameRule.ts @@ -15,6 +15,12 @@ export class Rule extends Lint.Rules.AbstractRule { hasFix: true, options: null, // tslint:disable-line:no-null-keyword optionsDescription: '', + optionExamples: [ + [true], + [true, { moduleName: 'yourModuleName' }], + [true, { moduleName: 'yourModuleName' }, ['module1', 'module2']], + [true, { moduleName: 'yourModuleName' }, ['module1', 'module2'], { ignoreExternalModule: false }] + ], typescriptOnly: true, issueClass: 'Ignored', issueType: 'Warning', From 32316cc48fcec3413f6526a2b40775f405227965 Mon Sep 17 00:00:00 2001 From: Taeheon Kim Date: Wed, 24 Oct 2018 02:57:21 +0900 Subject: [PATCH 13/13] Change optionsExamples --> naming of previous one can be confusing. --- src/importNameRule.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/importNameRule.ts b/src/importNameRule.ts index f2050993d..f295a6524 100644 --- a/src/importNameRule.ts +++ b/src/importNameRule.ts @@ -17,9 +17,9 @@ export class Rule extends Lint.Rules.AbstractRule { optionsDescription: '', optionExamples: [ [true], - [true, { moduleName: 'yourModuleName' }], - [true, { moduleName: 'yourModuleName' }, ['module1', 'module2']], - [true, { moduleName: 'yourModuleName' }, ['module1', 'module2'], { ignoreExternalModule: false }] + [true, { moduleName: 'importedName' }], + [true, { moduleName: 'importedName' }, ['moduleName1', 'moduleName2']], + [true, { moduleName: 'importedName' }, ['moduleName1', 'moduleName2'], { ignoreExternalModule: false }] ], typescriptOnly: true, issueClass: 'Ignored',