From 77647400676f5f7a7bca8fd18817dfa6bf0602a7 Mon Sep 17 00:00:00 2001 From: mgechev Date: Mon, 20 Feb 2017 16:12:59 -0800 Subject: [PATCH 1/5] feat: implement auto-removal of dead css --- src/angular/sourceMappingVisitor.ts | 16 +++++++++++++--- src/noUnusedCssRule.ts | 10 +++++++--- src/useLifeCycleInterfaceRule.ts | 2 +- 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/angular/sourceMappingVisitor.ts b/src/angular/sourceMappingVisitor.ts index 0e767538e..4aa86658f 100644 --- a/src/angular/sourceMappingVisitor.ts +++ b/src/angular/sourceMappingVisitor.ts @@ -1,5 +1,5 @@ import * as ts from 'typescript'; -import {RuleWalker, RuleFailure, IOptions} from 'tslint'; +import {RuleWalker, RuleFailure, IOptions, Fix, Replacement} from 'tslint'; import {ComponentMetadata, CodeWithSourceMap} from './metadata'; import {SourceMapConsumer} from 'source-map'; @@ -35,7 +35,17 @@ export class SourceMappingVisitor extends RuleWalker { super(sourceFile, options); } - createFailure(start: number, length: number, message: string): RuleFailure { + createFailure(s: number, l: number, message: string, fix?: Fix): RuleFailure { + const { start, length } = this.getMappedInterval(s, l); + return super.createFailure(start, length, message, fix); + } + + createReplacement(s: number, l: number, replacement: string): Replacement { + const { start, length } = this.getMappedInterval(s, l); + return super.createReplacement(start, length, replacement); + } + + private getMappedInterval(start: number, length: number) { let end = start + length; if (this.codeWithMap.map) { const consumer = new SourceMapConsumer(this.codeWithMap.map); @@ -45,7 +55,7 @@ export class SourceMappingVisitor extends RuleWalker { start += this.basePosition; end = start + length; } - return super.createFailure(start, end - start, message); + return { start, length: end - start }; } private getMappedPosition(pos: number, consumer: SourceMapConsumer) { diff --git a/src/noUnusedCssRule.ts b/src/noUnusedCssRule.ts index afebf5f88..eee3596a9 100644 --- a/src/noUnusedCssRule.ts +++ b/src/noUnusedCssRule.ts @@ -11,7 +11,7 @@ import { PropertyBindingType } from '@angular/compiler'; import {parseTemplate} from './angular/templates/templateParser'; -import {CssAst, CssSelectorRuleAst, CssSelectorAst} from './angular/styles/cssAst'; +import { CssAst, CssSelectorRuleAst, CssSelectorAst, CssBlockAst } from './angular/styles/cssAst'; import {ComponentMetadata, StyleMetadata} from './angular/metadata'; import {ng2WalkerFactoryUtils} from './angular/ng2WalkerFactoryUtils'; @@ -154,8 +154,12 @@ class UnusedCssVisitor extends BasicCssAstVisitor { try { const match = ast.selectors.some(s => this.visitCssSelector(s)); if (!match) { - this.addFailure(this.createFailure(ast.start.offset, - ast.end.offset - ast.start.offset, 'Unused styles')); + // We need this because of eventual source maps + const start = ast.start.offset; + const length = ast.end.offset - ast.start.offset; + // length + 1 because we want to drop the '}' + const fix = this.createFix(this.createReplacement(start, length + 1, '')); + this.addFailure(this.createFailure(start, length, 'Unused styles', fix)); } } catch (e) { logger.error(e); diff --git a/src/useLifeCycleInterfaceRule.ts b/src/useLifeCycleInterfaceRule.ts index 09008973d..94d8f78b0 100644 --- a/src/useLifeCycleInterfaceRule.ts +++ b/src/useLifeCycleInterfaceRule.ts @@ -57,7 +57,7 @@ export class ClassMetadataWalker extends Lint.RuleWalker { } private validateMethods( methods:any[],interfaces:string[],className:string){ - methods.forEach(m=>{ + methods.forEach(m => { let n = (m.name).text; if(n && this.isMethodValidHook(m,interfaces)){ let hookName = n.substr(2, n.lenght); From 8faa3a114fd3d917424a113ff780b539afd960ff Mon Sep 17 00:00:00 2001 From: mgechev Date: Mon, 20 Feb 2017 17:01:19 -0800 Subject: [PATCH 2/5] fix: proper source mapping --- src/angular/sourceMappingVisitor.ts | 144 ++++++++++++++++------ test/angular/sourceMappingVisitor.spec.ts | 8 +- test/noUnusedCssRule.spec.ts | 8 +- 3 files changed, 116 insertions(+), 44 deletions(-) diff --git a/src/angular/sourceMappingVisitor.ts b/src/angular/sourceMappingVisitor.ts index 4aa86658f..8d52e403a 100644 --- a/src/angular/sourceMappingVisitor.ts +++ b/src/angular/sourceMappingVisitor.ts @@ -3,36 +3,107 @@ import {RuleWalker, RuleFailure, IOptions, Fix, Replacement} from 'tslint'; import {ComponentMetadata, CodeWithSourceMap} from './metadata'; import {SourceMapConsumer} from 'source-map'; -const findLineAndColumnNumber = (pos: number, code: string) => { - code = code.replace('\r\n', '\n').replace('\r', '\n'); - let line = 1; - let column = 0; - for (let i = 0; i < pos; i += 1) { - column += 1; - if (code[i] === '\n') { - line += 1; - column = 0; +const LineFeed = 0x0A; +const CarriageReturn = 0x0D; +const MaxAsciiCharacter = 0x7F; +const LineSeparator = 0x2028; +const ParagraphSeparator = 0x2029; + +export function isLineBreak(ch: number): boolean { + return ch === LineFeed || + ch === CarriageReturn || + ch === LineSeparator || + ch === ParagraphSeparator; +} + +function binarySearch(array: T[], value: T, comparer?: (v1: T, v2: T) => number, offset?: number): number { + if (!array || array.length === 0) { + return -1; + } + + let low = offset || 0; + let high = array.length - 1; + comparer = comparer !== undefined + ? comparer + : (v1, v2) => (v1 < v2 ? -1 : (v1 > v2 ? 1 : 0)); + + while (low <= high) { + const middle = low + ((high - low) >> 1); + const midValue = array[middle]; + + if (comparer(midValue, value) === 0) { + return middle; + } else if (comparer(midValue, value) > 0) { + high = middle - 1; + } else { + low = middle + 1; } } - return { line, column }; -}; - -const findCharNumberFromLineAndColumn = ({ line, column }: { line: number, column: number }, code: string) => { - code = code.replace('\r\n', '\n').replace('\r', '\n'); - let char = 0; - while (line) { - if (code[char] === '\n') { - line -= 1; + + return ~low; +} + +// Apply caching and do not recompute every time +function getLineAndCharacterOfPosition(sourceFile: string, position: number) { + return computeLineAndCharacterOfPosition(computeLineStarts(sourceFile), position); +} + +// Apply caching and do not recompute every time +function getPositionOfLineAndCharacter(sourceFile: string, line: number, character: number): number { + return computePositionOfLineAndCharacter(computeLineStarts(sourceFile), line, character); +} + +function computePositionOfLineAndCharacter(lineStarts: number[], line: number, character: number): number { + return lineStarts[line] + character; +} + +function computeLineAndCharacterOfPosition(lineStarts: number[], position: number) { + let lineNumber = binarySearch(lineStarts, position); + if (lineNumber < 0) { + lineNumber = ~lineNumber - 1; + } + return { + line: lineNumber, + character: position - lineStarts[lineNumber] + }; +} + +function computeLineStarts(text: string): number[] { + const result: number[] = new Array(); + let pos = 0; + let lineStart = 0; + while (pos < text.length) { + const ch = text.charCodeAt(pos); + pos++; + switch (ch) { + case CarriageReturn: + if (text.charCodeAt(pos) === LineFeed) { + pos++; + } + case LineFeed: + result.push(lineStart); + lineStart = pos; + break; + default: + if (ch > MaxAsciiCharacter && isLineBreak(ch)) { + result.push(lineStart); + lineStart = pos; + } + break; } - char += 1; } - return char + column; -}; + result.push(lineStart); + return result; +} export class SourceMappingVisitor extends RuleWalker { + private consumer: SourceMapConsumer; constructor(sourceFile: ts.SourceFile, options: IOptions, protected codeWithMap: CodeWithSourceMap, protected basePosition: number) { super(sourceFile, options); + if (this.codeWithMap.map) { + this.consumer = new SourceMapConsumer(this.codeWithMap.map); + } } createFailure(s: number, l: number, message: string, fix?: Fix): RuleFailure { @@ -45,23 +116,24 @@ export class SourceMappingVisitor extends RuleWalker { return super.createReplacement(start, length, replacement); } - private getMappedInterval(start: number, length: number) { - let end = start + length; - if (this.codeWithMap.map) { - const consumer = new SourceMapConsumer(this.codeWithMap.map); - start = this.getMappedPosition(start, consumer); - end = this.getMappedPosition(end, consumer); - } else { - start += this.basePosition; - end = start + length; + getSourcePosition(pos: number) { + if (this.consumer) { + try { + let absPos = getLineAndCharacterOfPosition(this.codeWithMap.code, pos); + const result = this.consumer.originalPositionFor({ line: absPos.line + 1, column: absPos.character + 1 }); + absPos = { line: result.line - 1, character: result.column - 1 }; + pos = getPositionOfLineAndCharacter(this.codeWithMap.source, absPos.line, absPos.character); + } catch (e) { + console.log(e); + } } - return { start, length: end - start }; + return pos + this.basePosition; } - private getMappedPosition(pos: number, consumer: SourceMapConsumer) { - const absPos = findLineAndColumnNumber(pos, this.codeWithMap.code); - const mappedPos = consumer.originalPositionFor(absPos); - const char = findCharNumberFromLineAndColumn(mappedPos, this.codeWithMap.source); - return char + this.basePosition; + private getMappedInterval(start: number, length: number) { + let end = start + length; + start = this.getSourcePosition(start); + end = this.getSourcePosition(end); + return { start, length: end - start }; } } diff --git a/test/angular/sourceMappingVisitor.spec.ts b/test/angular/sourceMappingVisitor.spec.ts index d480cf4bd..30f04a692 100644 --- a/test/angular/sourceMappingVisitor.spec.ts +++ b/test/angular/sourceMappingVisitor.spec.ts @@ -25,7 +25,7 @@ const fixture1 = export class Foo {} `; -describe('metadataReader', () => { +describe('SourceMappingVisitor', () => { it('should map to correct position', () => { const ast = getAst(fixture1); @@ -39,8 +39,8 @@ describe('metadataReader', () => { map: JSON.parse(result.map.toString()), source: scss }, styleNode.getStart() + 1); - const failure = visitor.createFailure(0, 4, 'bar'); - chai.expect(failure.getStartPosition().getPosition()).eq(46); - chai.expect(failure.getEndPosition().getPosition()).eq(50); + const failure = visitor.createFailure(0, 3, 'bar'); + chai.expect(failure.getStartPosition().getPosition()).eq(34); + chai.expect(failure.getEndPosition().getPosition()).eq(38); }); }); diff --git a/test/noUnusedCssRule.spec.ts b/test/noUnusedCssRule.spec.ts index 4ec14b8d1..01f8c0969 100644 --- a/test/noUnusedCssRule.spec.ts +++ b/test/noUnusedCssRule.spec.ts @@ -737,12 +737,12 @@ describe('no-unused-css', () => { assertFailure('no-unused-css', source, { message: 'Unused styles', startPosition: { - line: 9, - character: 8 + line: 8, + character: 7 }, endPosition: { - line: 13, - character: 11 + line: 12, + character: 12 } }); Config.transformStyle = (code: string) => ({ code, map: null }); From 1096a41becd276bc4035ab4d80b2c25cb33ecef0 Mon Sep 17 00:00:00 2001 From: mgechev Date: Mon, 20 Feb 2017 17:24:49 -0800 Subject: [PATCH 3/5] fix: proper tests failure locations --- src/noUnusedCssRule.ts | 17 +++++++++++++++-- test/noUnusedCssRule.spec.ts | 22 +++++++++++----------- 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/src/noUnusedCssRule.ts b/src/noUnusedCssRule.ts index eee3596a9..41c8632ca 100644 --- a/src/noUnusedCssRule.ts +++ b/src/noUnusedCssRule.ts @@ -150,15 +150,28 @@ export class Rule extends Lint.Rules.AbstractRule { class UnusedCssVisitor extends BasicCssAstVisitor { templateAst: TemplateAst; + constructor(sourceFile: ts.SourceFile, + originalOptions: Lint.IOptions, + context: ComponentMetadata, + protected style: StyleMetadata, + templateStart: number) { + super(sourceFile, originalOptions, context, style, templateStart); + } + visitCssSelectorRule(ast: CssSelectorRuleAst) { try { const match = ast.selectors.some(s => this.visitCssSelector(s)); if (!match) { // We need this because of eventual source maps const start = ast.start.offset; - const length = ast.end.offset - ast.start.offset; + const end = ast.end.offset; + const length = end - ast.start.offset + 1; + let removeLength = length; // length + 1 because we want to drop the '}' - const fix = this.createFix(this.createReplacement(start, length + 1, '')); + if (/\s/.test(this.style.style.source[this.getSourcePosition(end) - this.style.node.getStart()])) { + removeLength += 1; + } + const fix = this.createFix(this.createReplacement(start, removeLength, '')); this.addFailure(this.createFailure(start, length, 'Unused styles', fix)); } } catch (e) { diff --git a/test/noUnusedCssRule.spec.ts b/test/noUnusedCssRule.spec.ts index 01f8c0969..08a76d361 100644 --- a/test/noUnusedCssRule.spec.ts +++ b/test/noUnusedCssRule.spec.ts @@ -233,7 +233,7 @@ describe('no-unused-css', () => { }, endPosition: { line: 12, - character: 12 + character: 13 } }); }); @@ -271,7 +271,7 @@ describe('no-unused-css', () => { }, endPosition: { line: 17, - character: 12 + character: 13 } }); }); @@ -304,7 +304,7 @@ describe('no-unused-css', () => { }, endPosition: { line: 12, - character: 12 + character: 13 } }); }); @@ -339,7 +339,7 @@ describe('no-unused-css', () => { }, endPosition: { line: 12, - character: 14 + character: 15 } }); }); @@ -399,7 +399,7 @@ describe('no-unused-css', () => { }, endPosition: { line: 12, - character: 12 + character: 13 } }); }); @@ -457,7 +457,7 @@ describe('no-unused-css', () => { }, endPosition: { line: 12, - character: 12 + character: 13 } }); }); @@ -513,7 +513,7 @@ describe('no-unused-css', () => { }, endPosition: { line: 12, - character: 12 + character: 13 } }); }); @@ -572,7 +572,7 @@ describe('no-unused-css', () => { }, endPosition: { line: 12, - character: 12 + character: 13 } }); }); @@ -638,7 +638,7 @@ describe('no-unused-css', () => { }, endPosition: { line: 9, - character: 12 + character: 13 } }); }); @@ -666,7 +666,7 @@ describe('no-unused-css', () => { }, endPosition: { line: 9, - character: 12 + character: 13 } }); }); @@ -694,7 +694,7 @@ describe('no-unused-css', () => { }, endPosition: { line: 9, - character: 12 + character: 13 } }); }); From fc43d7353bb9a8c2b281296068ff98176c451701 Mon Sep 17 00:00:00 2001 From: mgechev Date: Mon, 20 Feb 2017 17:43:26 -0800 Subject: [PATCH 4/5] fix: remove extra character removal --- src/noUnusedCssRule.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/noUnusedCssRule.ts b/src/noUnusedCssRule.ts index 41c8632ca..2fe0fb641 100644 --- a/src/noUnusedCssRule.ts +++ b/src/noUnusedCssRule.ts @@ -166,12 +166,8 @@ class UnusedCssVisitor extends BasicCssAstVisitor { const start = ast.start.offset; const end = ast.end.offset; const length = end - ast.start.offset + 1; - let removeLength = length; // length + 1 because we want to drop the '}' - if (/\s/.test(this.style.style.source[this.getSourcePosition(end) - this.style.node.getStart()])) { - removeLength += 1; - } - const fix = this.createFix(this.createReplacement(start, removeLength, '')); + const fix = this.createFix(this.createReplacement(start, length, '')); this.addFailure(this.createFailure(start, length, 'Unused styles', fix)); } } catch (e) { From 7df224da57482b732580d6e92c2a89841455530e Mon Sep 17 00:00:00 2001 From: mgechev Date: Mon, 20 Feb 2017 18:00:23 -0800 Subject: [PATCH 5/5] feat: add spec for fixes --- test/noUnusedCssRule.spec.ts | 93 ++++++++++++++++++++++++++++++++++++ test/testHelper.ts | 14 ++++-- 2 files changed, 103 insertions(+), 4 deletions(-) diff --git a/test/noUnusedCssRule.spec.ts b/test/noUnusedCssRule.spec.ts index 08a76d361..f45ef1c96 100644 --- a/test/noUnusedCssRule.spec.ts +++ b/test/noUnusedCssRule.spec.ts @@ -1,3 +1,4 @@ +import {expect} from 'chai'; import {Decorator} from 'typescript'; import * as sass from 'node-sass'; @@ -773,4 +774,96 @@ describe('no-unused-css', () => { }); + describe('autofixes', () => { + + it('should work with regular CSS', () => { + let source = ` + @Component({ + selector: 'foobar', + encapsulation: prefix.foo.ViewEncapsulation.Emulated, + template: \`
\`, + styles: [ + \` + p { + color: red; + } + \` + ] + }) + class Test {}`; + const failures = assertFailure('no-unused-css', source, { + message: 'Unused styles', + startPosition: { + line: 7, + character: 12 + }, + endPosition: { + line: 9, + character: 13 + } + }, null); + const fix = failures[0].getFix(); + const replacements = fix.replacements; + expect(replacements.length).to.eq(1); + const replacement = replacements[0]; + expect(replacement.text).to.eq(''); + expect(replacement.start).to.eq(197); + expect(replacement.end).to.eq(240); + }); + + it('should work with SASS', () => { + Config.transformStyle = (source: string, url: string, d: Decorator) => { + const res = sass.renderSync({ + sourceMap: true, data: source, sourceMapEmbed: true + }); + const code = res.css.toString(); + const base64Map = code.match(/\/\*(.*?)\*\//)[1].replace('# sourceMappingURL=data:application/json;base64,', ''); + const map = JSON.parse(new Buffer(base64Map, 'base64').toString('ascii')); + return { code, source, map }; + }; + + let source = ` + @Component({ + selector: 'hero-cmp', + template: \` +

Hello {{ hero.name }}

+ \`, + styles: [ + \` + h1 { + spam { + baz { + color: red; + } + } + } + \` + ] + }) + class HeroComponent { + private hero: Hero; + }`; + const failures = assertFailure('no-unused-css', source, { + message: 'Unused styles', + startPosition: { + line: 8, + character: 9 + }, + endPosition: { + line: 12, + character: 14 + } + }); + Config.transformStyle = (code: string) => ({ code, map: null }); + const fix = failures[0].getFix(); + const replacements = fix.replacements; + expect(replacements.length).to.eq(1); + const replacement = replacements[0]; + expect(replacement.text).to.eq(''); + expect(replacement.start).to.eq(174); + expect(replacement.end).to.eq(261); // should be 276 + }); + + }); + }); diff --git a/test/testHelper.ts b/test/testHelper.ts index ae83207eb..6619c67e6 100644 --- a/test/testHelper.ts +++ b/test/testHelper.ts @@ -1,4 +1,5 @@ import * as tslint from 'tslint'; +import * as Lint from 'tslint'; import chai = require('chai'); interface ISourcePosition { @@ -12,7 +13,7 @@ export interface IExpectedFailure { endPosition: ISourcePosition; } -function lint(ruleName: string, source: string, options): tslint.LintResult { +function lint(ruleName: string, source: string, options: any): tslint.LintResult { let configuration = { rules: {} }; @@ -95,8 +96,8 @@ export function assertAnnotated(config: AssertConfig) { } }; -export function assertFailure(ruleName: string, source: string, fail: IExpectedFailure, options = null) { - let result; +export function assertFailure(ruleName: string, source: string, fail: IExpectedFailure, options = null): Lint.RuleFailure[] { + let result: Lint.LintResult; try { result = lint(ruleName, source, options); } catch (e) { @@ -108,6 +109,10 @@ export function assertFailure(ruleName: string, source: string, fail: IExpectedF chai.assert.deepEqual(fail.startPosition, ruleFail.getStartPosition().getLineAndCharacter(), 'start char doesn\'t match'); chai.assert.deepEqual(fail.endPosition, ruleFail.getEndPosition().getLineAndCharacter(), 'end char doesn\'t match'); }); + if (result) { + return result.failures; + } + return undefined; }; export function assertFailures(ruleName: string, source: string, fails: IExpectedFailure[], options = null) { @@ -120,7 +125,8 @@ export function assertFailures(ruleName: string, source: string, fails: IExpecte chai.assert(result.failureCount > 0, 'no failures'); result.failures.forEach((ruleFail,index) => { chai.assert.equal(fails[index].message, ruleFail.getFailure(), 'error messages dont\'t match'); - chai.assert.deepEqual(fails[index].startPosition, ruleFail.getStartPosition().getLineAndCharacter(), 'start char doesn\'t match'); + chai.assert.deepEqual(fails[index].startPosition, ruleFail.getStartPosition().getLineAndCharacter(), + 'start char doesn\'t match'); chai.assert.deepEqual(fails[index].endPosition, ruleFail.getEndPosition().getLineAndCharacter(), 'end char doesn\'t match'); }); };