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

Commit

Permalink
fix(lint): improve linting performance
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
rolandjitsu authored and danbucholtz committed Jun 30, 2017
1 parent 27ac148 commit 106d82c
Show file tree
Hide file tree
Showing 9 changed files with 271 additions and 93 deletions.
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
"xml2js": "0.4.17"
},
"devDependencies": {
"@angular/animations": "4.1.3",
"@angular/common": "4.1.3",
"@angular/compiler": "4.1.3",
"@angular/compiler-cli": "4.1.3",
Expand Down
4 changes: 2 additions & 2 deletions src/generators/util.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ $TAB_CONTENT
const suppliedName = 'settings view';

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

// what we want to test
// what we want to test
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 }]);

// test
Expand Down
10 changes: 6 additions & 4 deletions src/lint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { access } from 'fs';
import { join } from 'path';

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

export function lintUpdateWorker(context: BuildContext, {tsConfig, tsLintConfig, filePaths, typeCheck}: LintWorkerConfig) {
const program = createProgram(context, tsConfig);
return getLintConfig(context, tsLintConfig)
.then(tsLintConfig => lintFiles(context, tsConfig, tsLintConfig, filePaths, {typeCheck}))
.then(tsLintConfig => lintFiles(context, program, tsLintConfig, filePaths, {typeCheck}))
// Don't throw if linting failed
.catch(() => {});
}


function lintApp(context: BuildContext, {tsConfig, tsLintConfig, typeCheck}: LintWorkerConfig) {
const files = getFileNames(context, tsConfig);
return lintFiles(context, tsConfig, tsLintConfig, files, {typeCheck});
const program = createProgram(context, tsConfig);
const files = getFileNames(context, program);
return lintFiles(context, program, tsLintConfig, files, {typeCheck});
}


Expand Down
159 changes: 159 additions & 0 deletions src/lint/lint-factory.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
import { Configuration, Linter } from 'tslint';
import { DiagnosticCategory } from 'typescript';
import * as ts from 'typescript';
import { isObject } from 'util';
import {
createLinter,
createProgram,
getFileNames,
getLintResult,
getTsLintConfig,
lint,
typeCheck
} from './lint-factory';


describe('lint factory', () => {
describe('createProgram()', () => {
it('should create a TS Program', () => {
const context: any = {rootDir: ''};
const program: any = createProgram(context, '');
const fns = [
'getSourceFiles',
'getTypeChecker'
];

expect(isObject(program)).toBeTruthy();
for (const fn of fns) {
expect(typeof program[fn]).toEqual('function');
}
});
});

describe('getTsLintConfig()', () => {
it('should fetch the TSLint configuration from file path', () => {
const tsConfigFilePath = 'tsconfig.json';
const mockConfig = {rulesDirectory: ['node_modules/@ionic']};
spyOn(Configuration, Configuration.loadConfigurationFromPath.name).and.returnValue(mockConfig);

const config = getTsLintConfig(tsConfigFilePath);

expect(isObject(config)).toBeTruthy();
expect(Configuration.loadConfigurationFromPath).toHaveBeenLastCalledWith(tsConfigFilePath);
expect(config).toEqual(mockConfig);
});

it('should extend configuration with {linterOptions} if provided', () => {
const tsConfigFilePath = 'tsconfig.json';
const mockConfig = {rulesDirectory: ['node_modules/@ionic']};
spyOn(Configuration, Configuration.loadConfigurationFromPath.name).and.returnValue(mockConfig);
const config = getTsLintConfig(tsConfigFilePath, {
typeCheck: true
});

expect(config.linterOptions).toEqual({
typeCheck: true
});
});
});

describe('createLinter()', () => {
it('should create a Linter', () => {
const context: any = {rootDir: ''};
const program = createProgram(context, '');
const linter = createLinter(context, program);

expect(linter instanceof Linter).toBeTruthy();
});
});

describe('getFileNames()', () => {
it('should get the file names referenced in the tsconfig.json', () => {
const context: any = {rootDir: ''};
const program = createProgram(context, '');
const mockFiles = ['test.ts'];
spyOn(Linter, 'getFileNames').and.returnValue(mockFiles);
const files = getFileNames(context, program);

expect(Array.isArray(files)).toBeTruthy();
expect(files).toEqual(mockFiles);
});
});

describe('typeCheck()', () => {
it('should not be called if {typeCheck} is false', done => {
const context: any = {rootDir: ''};
const program = createProgram(context, '');

spyOn(ts, ts.getPreEmitDiagnostics.name).and.returnValue([]);

typeCheck(context, program, {typeCheck: false})
.then((result) => {
expect(ts.getPreEmitDiagnostics).toHaveBeenCalledTimes(0);
expect(result).toEqual([]);
done();
});
});

it('should type check if {typeCheck} is true', done => {
const context: any = {rootDir: ''};
const program = createProgram(context, '');

const diagnostics: any = [{
file: {},
start: 2,
length: 10,
messageText: 'Oops',
category: DiagnosticCategory.Warning,
code: 120
}];

spyOn(ts, ts.getPreEmitDiagnostics.name).and.returnValue(diagnostics);

typeCheck(context, program, {typeCheck: true})
.then((result) => {
expect(ts.getPreEmitDiagnostics).toHaveBeenCalledWith(program);
expect(result).toEqual(diagnostics);
done();
});
});
});

describe('lint()', () => {
it('should lint a file', () => {
const context: any = {rootDir: ''};
const program = createProgram(context, '');
const linter = createLinter(context, program);
spyOn(linter, 'lint').and.returnValue(undefined);
const config = {};
const filePath = 'test.ts';
const fileContents = 'const test = true;';

lint(linter, config, filePath, fileContents);

expect(linter.lint).toHaveBeenCalledWith(filePath, fileContents, config);
});
});
describe('getLintResult()', () => {
it('should get the lint results after linting a file', () => {
const context: any = {rootDir: ''};
const program = createProgram(context, '');
const linter = createLinter(context, program);
spyOn(linter, 'lint').and.returnValue(undefined);
const mockResult: any = {};
spyOn(linter, 'getResult').and.returnValue(mockResult);
const config = {
jsRules: new Map(),
rules: new Map()
};
const filePath = 'test.ts';
const fileContents = 'const test = true;';

lint(linter, config, filePath, fileContents);

const result = getLintResult(linter);
expect(isObject(result)).toBeTruthy();
expect(result).toEqual(mockResult);
});
});
});
57 changes: 36 additions & 21 deletions src/lint/lint-factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,43 +8,49 @@ export interface LinterOptions {
typeCheck?: boolean;
}

export interface LinterConfig {
[key: string]: any;
}


/**
* Run linter on a file
* @param {BuildContext} context
* @param {string} tsLintConfig
* @param {string} tsConfig
* Lint a file according to config
* @param {Linter} linter
* @param {LinterConfig} config
* @param {string} filePath
* @param {string} fileContents
* @param {LinterOptions} linterOptions
*/
export function lint(linter: Linter, config: LinterConfig, filePath: string, fileContents: string): void {
linter.lint(filePath, fileContents, config as any);
}

/**
* Get the linter result
* @param {Linter} linter
* @return {LintResult}
*/
export function lint(context: BuildContext, tsConfig: string, tsLintConfig: string | null, filePath: string, fileContents: string, linterOptions?: LinterOptions): LintResult {
const linter = getLinter(context, tsConfig);
const configuration = Configuration.findConfiguration(tsLintConfig, filePath);
linter.lint(filePath, fileContents, Object.assign(configuration.results, isObject(linterOptions) ? {linterOptions} : {}));
export function getLintResult(linter: Linter): LintResult {
return linter.getResult();
}


/**
* Type check a TS program
* @param {BuildContext} context
* @param {string} tsConfig
* @param {Program} program
* @param {LinterOptions} linterOptions
* @return {Promise<Diagnostic[]>}
*/
export function typeCheck(context: BuildContext, tsConfig: string, linterOptions?: LinterOptions): Promise<Diagnostic[]> {
export function typeCheck(context: BuildContext, program: Program, linterOptions?: LinterOptions): Promise<Diagnostic[]> {
if (isObject(linterOptions) && linterOptions.typeCheck) {
const program = createProgram(context, tsConfig);
return Promise.resolve(getPreEmitDiagnostics(program));
}
return Promise.resolve([]);
}


/**
* Create a TS program based on the BuildContext {srcDir} or TS config file path (if provided)
* Create a TS program based on the BuildContext {rootDir} or TS config file path (if provided)
* @param {BuildContext} context
* @param {string} tsConfig
* @return {Program}
Expand All @@ -57,24 +63,33 @@ export function createProgram(context: BuildContext, tsConfig: string): Program
/**
* Get all files that are sourced in TS config
* @param {BuildContext} context
* @param {string} tsConfig
* @param {Program} program
* @return {Array<string>}
*/
export function getFileNames(context: BuildContext, tsConfig: string): string[] {
const program = createProgram(context, tsConfig);
export function getFileNames(context: BuildContext, program: Program): string[] {
return Linter.getFileNames(program);
}


/**
* Get linter
* @param {BuildContext} context
* @param {string} tsConfig
* Get lint configuration
* @param {string} tsLintConfig
* @param {LinterOptions} linterOptions
* @return {Linter}
*/
export function getLinter(context: BuildContext, tsConfig: string): Linter {
const program = createProgram(context, tsConfig);
export function getTsLintConfig(tsLintConfig: string, linterOptions?: LinterOptions): LinterConfig {
const config = Configuration.loadConfigurationFromPath(tsLintConfig);
Object.assign(config, isObject(linterOptions) ? {linterOptions} : {});
return config;
}

/**
* Create a TS linter
* @param {BuildContext} context
* @param {Program} program
* @return {Linter}
*/
export function createLinter(context: BuildContext, program: Program): Linter {
return new Linter({
fix: false
}, program);
Expand Down
Loading

0 comments on commit 106d82c

Please sign in to comment.