Skip to content
This repository was archived by the owner on May 1, 2020. It is now read-only.

Commit 106d82c

Browse files
rolandjitsudanbucholtz
rolandjitsu
authored and
danbucholtz
committed
fix(lint): improve linting performance
* chore: ignore WebStorm config files * chore: upgrade internal tslint dependencies - update tslint-eslint-rules - run tslint with --type-check flag * fix(interfaces): remove unnecessary semicolons * fix: remove unnecessary semicolons * chore: use stylish formatter for tslint * fix: fix tslint errors * chore: update mock-fs (allows using Node v7+) * feat(lint): upgrade to TSLint v5 * feat(lint): add ENV_TYPE_CHECK_ON_LINT and default to true * test(lint): cleanup a little * refactor(lint): improve linting logic * fix(lint): make CLI argument the same as the env var * feat(lint): add support for enabling type check during lint * test(lint): more cleanup * fix(lint): set project dir to context {rootDir} * chore: update tslint * chore(lint): add typeCheck() fn * chore: fix tslint errors * feat: implement and run type check if enabled * refactor(lint): rename generateFormattedErrorMsg() to generateErrorMessageForFiles() * fix(lint): filter duplicate file names * docs(lint): update note * chore(lint): add some TODOs * feat(lint): set default value for --typeCheck to false * docs(lint): document --typeCheck flag * fix(lint): don't create a new instance of BuildError when throwing after linting failed * fix(lint): set --typeCheckOnLint to null as default instead of false * test(lint): fix typeCheckOnLint flag tests * test(lint): add tests for generateErrorMessageForFiles(), getFileNames() and removeDuplicateFileNames() * chore(lint): remove some TODOs * fix(lint): properly pass CLI arguments to linter * feat(lint): properly disable type check during builds * fix(lint): allow null TSLint config file * chore: revert string change * chore: fix npm warning * chore: upgrade tslint-eslint-rules * perf(lint): only create the linter and TS program once per linting instance instead of once per file * chore: fix tslint errors * chore(lint): only fetch lint result after all files have been linted - rename processLintResults() to processLintResult() - update jsdocs * test(lint): test all lint fns * chore: fix tslint errors * refactor(lint): use Set to filter unique file names
1 parent 27ac148 commit 106d82c

File tree

9 files changed

+271
-93
lines changed

9 files changed

+271
-93
lines changed

‎package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@
6363
"xml2js": "0.4.17"
6464
},
6565
"devDependencies": {
66+
"@angular/animations": "4.1.3",
6667
"@angular/common": "4.1.3",
6768
"@angular/compiler": "4.1.3",
6869
"@angular/compiler-cli": "4.1.3",

‎src/generators/util.spec.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -410,7 +410,7 @@ $TAB_CONTENT
410410
const suppliedName = 'settings view';
411411

412412
it('should return a succesful promise', () => {
413-
// set up spies
413+
// set up spies
414414
spyOn(helpers, helpers.readFileAsync.name).and.returnValue(Promise.resolve('file content'));
415415
spyOn(fs, 'readdirSync').and.returnValue([
416416
join(process.cwd(), 'path', 'to', 'nowhere'),
@@ -420,7 +420,7 @@ $TAB_CONTENT
420420
spyOn(TypeScriptUtils, TypeScriptUtils.insertNamedImportIfNeeded.name).and.returnValue('file content');
421421
spyOn(TypeScriptUtils, TypeScriptUtils.appendNgModuleDeclaration.name).and.returnValue('sliced string');
422422

423-
// what we want to test
423+
// what we want to test
424424
const promise = util.tabsModuleManipulation([['/src/pages/cool-tab-one/cool-tab-one.module.ts']], { name: suppliedName, className: className, fileName: fileName }, [{ name: suppliedName, className: className, fileName: fileName }]);
425425

426426
// test

‎src/lint.ts

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { access } from 'fs';
22
import { join } from 'path';
33

44
import { lintFiles } from './lint/lint-utils';
5-
import { getFileNames } from './lint/lint-factory';
5+
import { createProgram, getFileNames } from './lint/lint-factory';
66
import { Logger } from './logger/logger';
77
import { getUserConfigFile } from './util/config';
88
import { ENV_BAIL_ON_LINT_ERROR, ENV_TYPE_CHECK_ON_LINT } from './util/constants';
@@ -64,16 +64,18 @@ export function lintUpdate(changedFiles: ChangedFile[], context: BuildContext, t
6464
}
6565

6666
export function lintUpdateWorker(context: BuildContext, {tsConfig, tsLintConfig, filePaths, typeCheck}: LintWorkerConfig) {
67+
const program = createProgram(context, tsConfig);
6768
return getLintConfig(context, tsLintConfig)
68-
.then(tsLintConfig => lintFiles(context, tsConfig, tsLintConfig, filePaths, {typeCheck}))
69+
.then(tsLintConfig => lintFiles(context, program, tsLintConfig, filePaths, {typeCheck}))
6970
// Don't throw if linting failed
7071
.catch(() => {});
7172
}
7273

7374

7475
function lintApp(context: BuildContext, {tsConfig, tsLintConfig, typeCheck}: LintWorkerConfig) {
75-
const files = getFileNames(context, tsConfig);
76-
return lintFiles(context, tsConfig, tsLintConfig, files, {typeCheck});
76+
const program = createProgram(context, tsConfig);
77+
const files = getFileNames(context, program);
78+
return lintFiles(context, program, tsLintConfig, files, {typeCheck});
7779
}
7880

7981

‎src/lint/lint-factory.spec.ts

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,159 @@
1+
import { Configuration, Linter } from 'tslint';
2+
import { DiagnosticCategory } from 'typescript';
3+
import * as ts from 'typescript';
4+
import { isObject } from 'util';
5+
import {
6+
createLinter,
7+
createProgram,
8+
getFileNames,
9+
getLintResult,
10+
getTsLintConfig,
11+
lint,
12+
typeCheck
13+
} from './lint-factory';
14+
15+
16+
describe('lint factory', () => {
17+
describe('createProgram()', () => {
18+
it('should create a TS Program', () => {
19+
const context: any = {rootDir: ''};
20+
const program: any = createProgram(context, '');
21+
const fns = [
22+
'getSourceFiles',
23+
'getTypeChecker'
24+
];
25+
26+
expect(isObject(program)).toBeTruthy();
27+
for (const fn of fns) {
28+
expect(typeof program[fn]).toEqual('function');
29+
}
30+
});
31+
});
32+
33+
describe('getTsLintConfig()', () => {
34+
it('should fetch the TSLint configuration from file path', () => {
35+
const tsConfigFilePath = 'tsconfig.json';
36+
const mockConfig = {rulesDirectory: ['node_modules/@ionic']};
37+
spyOn(Configuration, Configuration.loadConfigurationFromPath.name).and.returnValue(mockConfig);
38+
39+
const config = getTsLintConfig(tsConfigFilePath);
40+
41+
expect(isObject(config)).toBeTruthy();
42+
expect(Configuration.loadConfigurationFromPath).toHaveBeenLastCalledWith(tsConfigFilePath);
43+
expect(config).toEqual(mockConfig);
44+
});
45+
46+
it('should extend configuration with {linterOptions} if provided', () => {
47+
const tsConfigFilePath = 'tsconfig.json';
48+
const mockConfig = {rulesDirectory: ['node_modules/@ionic']};
49+
spyOn(Configuration, Configuration.loadConfigurationFromPath.name).and.returnValue(mockConfig);
50+
const config = getTsLintConfig(tsConfigFilePath, {
51+
typeCheck: true
52+
});
53+
54+
expect(config.linterOptions).toEqual({
55+
typeCheck: true
56+
});
57+
});
58+
});
59+
60+
describe('createLinter()', () => {
61+
it('should create a Linter', () => {
62+
const context: any = {rootDir: ''};
63+
const program = createProgram(context, '');
64+
const linter = createLinter(context, program);
65+
66+
expect(linter instanceof Linter).toBeTruthy();
67+
});
68+
});
69+
70+
describe('getFileNames()', () => {
71+
it('should get the file names referenced in the tsconfig.json', () => {
72+
const context: any = {rootDir: ''};
73+
const program = createProgram(context, '');
74+
const mockFiles = ['test.ts'];
75+
spyOn(Linter, 'getFileNames').and.returnValue(mockFiles);
76+
const files = getFileNames(context, program);
77+
78+
expect(Array.isArray(files)).toBeTruthy();
79+
expect(files).toEqual(mockFiles);
80+
});
81+
});
82+
83+
describe('typeCheck()', () => {
84+
it('should not be called if {typeCheck} is false', done => {
85+
const context: any = {rootDir: ''};
86+
const program = createProgram(context, '');
87+
88+
spyOn(ts, ts.getPreEmitDiagnostics.name).and.returnValue([]);
89+
90+
typeCheck(context, program, {typeCheck: false})
91+
.then((result) => {
92+
expect(ts.getPreEmitDiagnostics).toHaveBeenCalledTimes(0);
93+
expect(result).toEqual([]);
94+
done();
95+
});
96+
});
97+
98+
it('should type check if {typeCheck} is true', done => {
99+
const context: any = {rootDir: ''};
100+
const program = createProgram(context, '');
101+
102+
const diagnostics: any = [{
103+
file: {},
104+
start: 2,
105+
length: 10,
106+
messageText: 'Oops',
107+
category: DiagnosticCategory.Warning,
108+
code: 120
109+
}];
110+
111+
spyOn(ts, ts.getPreEmitDiagnostics.name).and.returnValue(diagnostics);
112+
113+
typeCheck(context, program, {typeCheck: true})
114+
.then((result) => {
115+
expect(ts.getPreEmitDiagnostics).toHaveBeenCalledWith(program);
116+
expect(result).toEqual(diagnostics);
117+
done();
118+
});
119+
});
120+
});
121+
122+
describe('lint()', () => {
123+
it('should lint a file', () => {
124+
const context: any = {rootDir: ''};
125+
const program = createProgram(context, '');
126+
const linter = createLinter(context, program);
127+
spyOn(linter, 'lint').and.returnValue(undefined);
128+
const config = {};
129+
const filePath = 'test.ts';
130+
const fileContents = 'const test = true;';
131+
132+
lint(linter, config, filePath, fileContents);
133+
134+
expect(linter.lint).toHaveBeenCalledWith(filePath, fileContents, config);
135+
});
136+
});
137+
describe('getLintResult()', () => {
138+
it('should get the lint results after linting a file', () => {
139+
const context: any = {rootDir: ''};
140+
const program = createProgram(context, '');
141+
const linter = createLinter(context, program);
142+
spyOn(linter, 'lint').and.returnValue(undefined);
143+
const mockResult: any = {};
144+
spyOn(linter, 'getResult').and.returnValue(mockResult);
145+
const config = {
146+
jsRules: new Map(),
147+
rules: new Map()
148+
};
149+
const filePath = 'test.ts';
150+
const fileContents = 'const test = true;';
151+
152+
lint(linter, config, filePath, fileContents);
153+
154+
const result = getLintResult(linter);
155+
expect(isObject(result)).toBeTruthy();
156+
expect(result).toEqual(mockResult);
157+
});
158+
});
159+
});

‎src/lint/lint-factory.ts

Lines changed: 36 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -8,43 +8,49 @@ export interface LinterOptions {
88
typeCheck?: boolean;
99
}
1010

11+
export interface LinterConfig {
12+
[key: string]: any;
13+
}
14+
1115

1216
/**
13-
* Run linter on a file
14-
* @param {BuildContext} context
15-
* @param {string} tsLintConfig
16-
* @param {string} tsConfig
17+
* Lint a file according to config
18+
* @param {Linter} linter
19+
* @param {LinterConfig} config
1720
* @param {string} filePath
1821
* @param {string} fileContents
19-
* @param {LinterOptions} linterOptions
22+
*/
23+
export function lint(linter: Linter, config: LinterConfig, filePath: string, fileContents: string): void {
24+
linter.lint(filePath, fileContents, config as any);
25+
}
26+
27+
/**
28+
* Get the linter result
29+
* @param {Linter} linter
2030
* @return {LintResult}
2131
*/
22-
export function lint(context: BuildContext, tsConfig: string, tsLintConfig: string | null, filePath: string, fileContents: string, linterOptions?: LinterOptions): LintResult {
23-
const linter = getLinter(context, tsConfig);
24-
const configuration = Configuration.findConfiguration(tsLintConfig, filePath);
25-
linter.lint(filePath, fileContents, Object.assign(configuration.results, isObject(linterOptions) ? {linterOptions} : {}));
32+
export function getLintResult(linter: Linter): LintResult {
2633
return linter.getResult();
2734
}
2835

2936

3037
/**
3138
* Type check a TS program
3239
* @param {BuildContext} context
33-
* @param {string} tsConfig
40+
* @param {Program} program
3441
* @param {LinterOptions} linterOptions
3542
* @return {Promise<Diagnostic[]>}
3643
*/
37-
export function typeCheck(context: BuildContext, tsConfig: string, linterOptions?: LinterOptions): Promise<Diagnostic[]> {
44+
export function typeCheck(context: BuildContext, program: Program, linterOptions?: LinterOptions): Promise<Diagnostic[]> {
3845
if (isObject(linterOptions) && linterOptions.typeCheck) {
39-
const program = createProgram(context, tsConfig);
4046
return Promise.resolve(getPreEmitDiagnostics(program));
4147
}
4248
return Promise.resolve([]);
4349
}
4450

4551

4652
/**
47-
* Create a TS program based on the BuildContext {srcDir} or TS config file path (if provided)
53+
* Create a TS program based on the BuildContext {rootDir} or TS config file path (if provided)
4854
* @param {BuildContext} context
4955
* @param {string} tsConfig
5056
* @return {Program}
@@ -57,24 +63,33 @@ export function createProgram(context: BuildContext, tsConfig: string): Program
5763
/**
5864
* Get all files that are sourced in TS config
5965
* @param {BuildContext} context
60-
* @param {string} tsConfig
66+
* @param {Program} program
6167
* @return {Array<string>}
6268
*/
63-
export function getFileNames(context: BuildContext, tsConfig: string): string[] {
64-
const program = createProgram(context, tsConfig);
69+
export function getFileNames(context: BuildContext, program: Program): string[] {
6570
return Linter.getFileNames(program);
6671
}
6772

6873

6974
/**
70-
* Get linter
71-
* @param {BuildContext} context
72-
* @param {string} tsConfig
75+
* Get lint configuration
76+
* @param {string} tsLintConfig
77+
* @param {LinterOptions} linterOptions
7378
* @return {Linter}
7479
*/
75-
export function getLinter(context: BuildContext, tsConfig: string): Linter {
76-
const program = createProgram(context, tsConfig);
80+
export function getTsLintConfig(tsLintConfig: string, linterOptions?: LinterOptions): LinterConfig {
81+
const config = Configuration.loadConfigurationFromPath(tsLintConfig);
82+
Object.assign(config, isObject(linterOptions) ? {linterOptions} : {});
83+
return config;
84+
}
7785

86+
/**
87+
* Create a TS linter
88+
* @param {BuildContext} context
89+
* @param {Program} program
90+
* @return {Linter}
91+
*/
92+
export function createLinter(context: BuildContext, program: Program): Linter {
7893
return new Linter({
7994
fix: false
8095
}, program);

0 commit comments

Comments
 (0)