`import-name` 6.2.0 ignoreExternalModules non-external modules that aren't TS or JS crash

dosentmatter commented 5 years ago

Bug Report

tslint-microsoft-contrib version: 6.2.0
TSLint version: 5.18.0
TypeScript version: 3.5.3
Running TSLint via: CLI

TypeScript code being linted

All the local files imported can just be empty.

import React from 'react'; // Doesn't cause an error
import blahSVG from './blah.svg'; // Errors
import fooSvg from './foo.svg'; // Doesn't cause an error. It doesn't go into checkIgnoreExternalModule() for some reason
import { blah } from './blah'; // blah.ts file

with tslint.json configuration:

{
  "rulesDirectory": ["tslint-microsoft-contrib"],
  "rules": {
    "import-name": true
  }
}

tsconfig.json. It needs to be passed to tslint -p, but an empty one is fine.

{
}

Actual behavior

I get this error

$ npx tslint -p tsconfig.json test.ts
The 'import-name' rule threw an error in '/Users/kevinlau/workspace/test/tslint-microsoft-contrib/test.ts':
TypeError: Cannot read property 'isExternalLibraryImport' of undefined
    at /Users/kevinlau/workspace/test/tslint-microsoft-contrib/node_modules/tslint-microsoft-contrib/importNameRule.js:127:49
    at Map.forEach (<anonymous>)
    at checkIgnoreExternalModule (/Users/kevinlau/workspace/test/tslint-microsoft-contrib/node_modules/tslint-microsoft-contrib/importNameRule.js:126:48)
    at isImportNameValid (/Users/kevinlau/workspace/test/tslint-microsoft-contrib/node_modules/tslint-microsoft-contrib/importNameRule.js:166:40)
    at validateImport (/Users/kevinlau/workspace/test/tslint-microsoft-contrib/node_modules/tslint-microsoft-contrib/importNameRule.js:197:13)
    at cb (/Users/kevinlau/workspace/test/tslint-microsoft-contrib/node_modules/tslint-microsoft-contrib/importNameRule.js:232:21)
    at visitNodes (/Users/kevinlau/workspace/test/tslint-microsoft-contrib/node_modules/typescript/lib/typescript.js:16631:30)
    at Object.forEachChild (/Users/kevinlau/workspace/test/tslint-microsoft-contrib/node_modules/typescript/lib/typescript.js:16859:24)
    at walk (/Users/kevinlau/workspace/test/tslint-microsoft-contrib/node_modules/tslint-microsoft-contrib/importNameRule.js:238:15)
    at Rule.AbstractRule.applyWithFunction (/Users/kevinlau/workspace/test/tslint-microsoft-contrib/node_modules/tslint/lib/language/rule/abstractRule.js:39:9)

Expected behavior

It should not crash. It should just have a lint error on blahSVG -> blahSvg.

In a debugger, you can see that the resolvedModules values are undefined for non TS or JS files. It fails when accessing. value.isExternalLibraryImport since value is undefined.

source lines

TS source says that ResolvedModule only picks up TS and JS files

I'm not sure why import fooSvg from './foo.svg'; doesn't crash though. checkIgnoreExternalModule() doesn't seem to be called on names that are named correctly. I haven't read all of your source code, but maybe you guys are doing a basic check for things already named in camelCase.

Another thing is that the crash doesn't count as a lint error so lint still passes.

IllusionMH commented 5 years ago

@dosentmatter thanks for the report. From static check standpoint it is impossible to tell if import "foo.svg" is import from foo.svg which is not supported by TS itself or from foo.svg.ts which is valid option.

Do you have types.d.ts file or similar with declaration like code below?

declare module "*.svg" {
  // declaration for processed SVG type
}

While Webpack (I assume you use it) can resolve correctly various extension TS doesn't process anything except TS/JS/JSON so this is probably root cause of difference.

Anyway this shouldn't be runtime error.

UPD. Removed incorrect mention of Webpack's resolve.extensions configuration

dosentmatter commented 5 years ago

Hey @IllusionMH, no problem. I didn't add a types.d.ts to my minimal example, but I just did and it doesn't change anything.

In the original project I was working on, it had a types.d.ts, but still had the error. Yeah, I am using webpack svg loaders for storybook, rollup svg plugins for bundling, and jest svg ignore transforms for tests.

Adding types.d.ts for my minimal example:

declare module "*.svg" {
  const a: string;
  export default a;
}

Here is the type showing in editor:

$ npx tslint -p tsconfig.json test.ts
The 'import-name' rule threw an error in '/Users/kevinlau/workspace/test/tslint-microsoft-contrib/test.ts':
TypeError: Cannot read property 'isExternalLibraryImport' of undefined
    at /Users/kevinlau/workspace/test/tslint-microsoft-contrib/node_modules/tslint-microsoft-contrib/importNameRule.js:127:49
    at Map.forEach (<anonymous>)
    at checkIgnoreExternalModule (/Users/kevinlau/workspace/test/tslint-microsoft-contrib/node_modules/tslint-microsoft-contrib/importNameRule.js:126:48)
    at isImportNameValid (/Users/kevinlau/workspace/test/tslint-microsoft-contrib/node_modules/tslint-microsoft-contrib/importNameRule.js:166:40)
    at validateImport (/Users/kevinlau/workspace/test/tslint-microsoft-contrib/node_modules/tslint-microsoft-contrib/importNameRule.js:197:13)
    at cb (/Users/kevinlau/workspace/test/tslint-microsoft-contrib/node_modules/tslint-microsoft-contrib/importNameRule.js:232:21)
    at visitNodes (/Users/kevinlau/workspace/test/tslint-microsoft-contrib/node_modules/typescript/lib/typescript.js:16631:30)
    at Object.forEachChild (/Users/kevinlau/workspace/test/tslint-microsoft-contrib/node_modules/typescript/lib/typescript.js:16859:24)
    at walk (/Users/kevinlau/workspace/test/tslint-microsoft-contrib/node_modules/tslint-microsoft-contrib/importNameRule.js:238:15)
    at Rule.AbstractRule.applyWithFunction (/Users/kevinlau/workspace/test/tslint-microsoft-contrib/node_modules/tslint/lib/language/rule/abstractRule.js:39:9)

JoshuaKGoldberg commented 4 years ago

💀 It's time! 💀

TSLint is deprecated and no longer accepting pull requests other than security fixes. See #876. ☠️ We recommend you instead use typescript-eslint to lint your TypeScript code with ESLint. ✅

👋 It was a pleasure open sourcing with you!

microsoft / tslint-microsoft-contrib