Skip to content

Commit 2ffe2ea

Browse files
rafaelss95mgechev
rafaelss95
authored and
mgechev
committedJul 29, 2018
fix(templates-no-negated-async): not reporting failures for some cases (#694)
·
6.0.14.4.3
1 parent 5a84041 commit 2ffe2ea

File tree

2 files changed

+152
-144
lines changed

2 files changed

+152
-144
lines changed
 

‎src/templatesNoNegatedAsyncRule.ts

+47-52
Original file line numberDiff line numberDiff line change
@@ -1,78 +1,73 @@
1-
import * as Lint from 'tslint';
2-
import * as ts from 'typescript';
1+
import { IRuleMetadata, RuleFailure, Rules } from 'tslint';
2+
import { SourceFile } from 'typescript';
33
import { NgWalker } from './angular/ngWalker';
44
import { RecursiveAngularExpressionVisitor } from './angular/templates/recursiveAngularExpressionVisitor';
5-
import * as e from '@angular/compiler/src/expression_parser/ast';
6-
import * as ast from '@angular/compiler';
5+
import { Binary, PrefixNot } from '@angular/compiler/src/expression_parser/ast';
6+
import { AST, BindingPipe, LiteralPrimitive } from '@angular/compiler';
77

88
const unstrictEqualityOperator = '==';
99

10-
class TemplateToNgTemplateVisitor extends RecursiveAngularExpressionVisitor {
11-
visitBinary(expr: e.Binary, context: any): any {
12-
if (!this.isAsyncBinding(expr.left)) {
13-
return super.visitBinary(expr, context);
14-
}
15-
if (!(expr.right instanceof ast.LiteralPrimitive) || expr.right.value !== false || expr.operation !== unstrictEqualityOperator) {
16-
return super.visitBinary(expr, context);
17-
}
10+
const isAsyncBinding = (ast: AST): boolean => {
11+
return ast instanceof BindingPipe && ast.name === 'async';
12+
};
1813

19-
const {
20-
left: {
21-
span: { end: endLeftSpan }
22-
},
23-
right: {
24-
span: { start: startRightSpan }
25-
},
26-
span: { end: spanEnd, start: spanStart }
27-
} = expr;
28-
const operator = this.codeWithMap.code.slice(endLeftSpan, startRightSpan);
29-
const operatorStart = /^.*==/.exec(operator)![0].length - unstrictEqualityOperator.length;
14+
class TemplateToNgTemplateVisitor extends RecursiveAngularExpressionVisitor {
15+
visitBinary(ast: Binary, context: any): any {
16+
this.validateBinary(ast);
17+
super.visitBinary(ast, context);
18+
}
3019

31-
this.addFailureFromStartToEnd(spanStart, spanEnd, 'Async pipes must use strict equality `===` when comparing with `false`', [
32-
new Lint.Replacement(this.getSourcePosition(endLeftSpan) + operatorStart, unstrictEqualityOperator.length, '===')
33-
]);
34-
super.visitBinary(expr, context);
20+
visitPrefixNot(ast: PrefixNot, context: any): any {
21+
this.validatePrefixNot(ast);
22+
super.visitPrefixNot(ast, context);
3523
}
3624

37-
visitPrefixNot(expr: e.PrefixNot, context: any): any {
38-
if (!this.isAsyncBinding(expr.expression)) {
39-
return super.visitPrefixNot(expr, context);
25+
private validateBinary(ast: Binary): void {
26+
const { left, operation, right } = ast;
27+
28+
if (!isAsyncBinding(left) || !(right instanceof LiteralPrimitive) || right.value !== false || operation !== unstrictEqualityOperator) {
29+
return;
4030
}
41-
const {
42-
span: { end: spanEnd, start: spanStart }
43-
} = expr;
44-
const absoluteStart = this.getSourcePosition(spanStart);
4531

46-
// Angular includes the whitespace after an expression, we want to trim that
47-
const expressionSource = this.codeWithMap.code.slice(spanStart, spanEnd);
48-
const concreteWidth = spanEnd - spanStart - / *$/.exec(expressionSource)![0].length;
32+
this.generateFailure(ast, Rule.FAILURE_STRING_UNSTRICT_EQUALITY);
33+
}
34+
35+
private validatePrefixNot(ast: PrefixNot): void {
36+
const { expression } = ast;
37+
38+
if (!isAsyncBinding(expression)) {
39+
return;
40+
}
4941

50-
this.addFailureFromStartToEnd(spanStart, spanEnd, 'Async pipes can not be negated, use (observable | async) === false instead', [
51-
new Lint.Replacement(absoluteStart + concreteWidth, 1, ' === false '),
52-
new Lint.Replacement(absoluteStart, 1, '')
53-
]);
42+
this.generateFailure(ast, Rule.FAILURE_STRING_NEGATED_PIPE);
5443
}
5544

56-
protected isAsyncBinding(expr: ast.AST) {
57-
return expr instanceof ast.BindingPipe && expr.name === 'async';
45+
private generateFailure(ast: Binary | PrefixNot, errorMessage: string): void {
46+
const {
47+
span: { end: spanEnd, start: spanStart }
48+
} = ast;
49+
50+
this.addFailureFromStartToEnd(spanStart, spanEnd, errorMessage);
5851
}
5952
}
6053

61-
export class Rule extends Lint.Rules.AbstractRule {
62-
public static metadata: Lint.IRuleMetadata = {
63-
ruleName: 'templates-no-negated-async',
64-
type: 'functionality',
54+
export class Rule extends Rules.AbstractRule {
55+
static readonly metadata: IRuleMetadata = {
6556
description: 'Ensures that strict equality is used when evaluating negations on async pipe output.',
57+
options: null,
58+
optionsDescription: 'Not configurable.',
6659
rationale:
6760
'Async pipe evaluate to `null` before the observable or promise emits, which can lead to layout thrashing as' +
6861
' components load. Prefer strict `=== false` checks instead.',
69-
options: null,
70-
optionsDescription: 'Not configurable.',
71-
typescriptOnly: true,
72-
hasFix: true
62+
ruleName: 'templates-no-negated-async',
63+
type: 'functionality',
64+
typescriptOnly: true
7365
};
7466

75-
public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
67+
static readonly FAILURE_STRING_NEGATED_PIPE = 'Async pipes can not be negated, use (observable | async) === false instead';
68+
static readonly FAILURE_STRING_UNSTRICT_EQUALITY = 'Async pipes must use strict equality `===` when comparing with `false`';
69+
70+
apply(sourceFile: SourceFile): RuleFailure[] {
7671
return this.applyWithWalker(
7772
new NgWalker(sourceFile, this.getOptions(), {
7873
expressionVisitorCtrl: TemplateToNgTemplateVisitor

‎test/templatesNoNegatedAsyncRule.spec.ts

+105-92
Original file line numberDiff line numberDiff line change
@@ -1,169 +1,182 @@
1-
import { assertSuccess, assertAnnotated } from './testHelper';
2-
import { Replacement } from 'tslint';
3-
import { expect } from 'chai';
1+
import { assertAnnotated, assertMultipleAnnotated, assertSuccess } from './testHelper';
2+
import { Rule } from '../src/templatesNoNegatedAsyncRule';
43

5-
describe('templates-no-negated-async', () => {
6-
describe('invalid expressions', () => {
7-
it('should fail when an async pipe is negated', () => {
8-
let source = `
4+
const {
5+
FAILURE_STRING_NEGATED_PIPE,
6+
FAILURE_STRING_UNSTRICT_EQUALITY,
7+
metadata: { ruleName }
8+
} = Rule;
9+
10+
describe(ruleName, () => {
11+
describe('failure', () => {
12+
it('should fail if async pipe is negated', () => {
13+
const source = `
914
@Component({
10-
selector: 'foobar',
15+
selector: 'test',
1116
template: '{{ !(foo | async) }}'
1217
~~~~~~~~~~~~~~~
1318
})
1419
class Test {
15-
constructor(public foo: Observable<Boolean>) {}
20+
constructor(foo: Observable<Boolean>) {}
1621
}
1722
`;
1823
assertAnnotated({
19-
ruleName: 'templates-no-negated-async',
20-
message: 'Async pipes can not be negated, use (observable | async) === false instead',
24+
message: FAILURE_STRING_NEGATED_PIPE,
25+
ruleName: ruleName,
2126
source
2227
});
2328
});
2429

25-
it('should fail when an async pipe is including other pipes', () => {
26-
let source = `
30+
it('should fail if async pipe is the last pipe in the negated chain', () => {
31+
const source = `
2732
@Component({
28-
selector: 'foobar',
33+
selector: 'test',
2934
template: '{{ !(foo | somethingElse | async) }}'
3035
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3136
})
3237
class Test {
33-
constructor(public foo: Observable<Boolean>) {}
38+
constructor(foo: Observable<Boolean>) {}
3439
}
3540
`;
3641
assertAnnotated({
37-
ruleName: 'templates-no-negated-async',
38-
message: 'Async pipes can not be negated, use (observable | async) === false instead',
42+
message: FAILURE_STRING_NEGATED_PIPE,
43+
ruleName: ruleName,
3944
source
4045
});
4146
});
4247

43-
it('should fail when an async pipe uses non-strict equality', () => {
44-
let source = `
48+
it('should fail if async pipe uses unstrict equality', () => {
49+
const source = `
4550
@Component({
46-
selector: 'foobar',
51+
selector: 'test',
4752
template: '{{ (foo | async) == false }}'
4853
~~~~~~~~~~~~~~~~~~~~~~
4954
})
5055
class Test {
51-
constructor(public foo: Observable<Boolean>) {}
56+
constructor(foo: Observable<Boolean>) {}
5257
}
5358
`;
5459
assertAnnotated({
55-
ruleName: 'templates-no-negated-async',
56-
message: 'Async pipes must use strict equality `===` when comparing with `false`',
60+
message: FAILURE_STRING_UNSTRICT_EQUALITY,
61+
ruleName: ruleName,
5762
source
5863
});
5964
});
6065

61-
describe('fixes', () => {
62-
it('fixes negated pipes', () => {
63-
let source = `
64-
@Component({
65-
selector: 'foobar',
66-
template: '{{ !(foo | async) }}'
67-
~~~~~~~~~~~~~~~
68-
})
69-
class Test {}
70-
`;
71-
const failures = assertAnnotated({
72-
ruleName: 'templates-no-negated-async',
73-
message: 'Async pipes can not be negated, use (observable | async) === false instead',
74-
source
75-
});
76-
77-
const res = Replacement.applyAll(source, failures[0].getFix());
78-
expect(res).to.eq(`
79-
@Component({
80-
selector: 'foobar',
81-
template: '{{ (foo | async) === false }}'
82-
~~~~~~~~~~~~~~~
83-
})
84-
class Test {}
85-
`);
66+
it('should fail if async pipe is negated using *ngIf', () => {
67+
const source = `
68+
@Component({
69+
selector: 'test',
70+
template: '<div *ngIf="!(a | async)"></div>'
71+
~~~~~~~~~~~~
72+
})
73+
class Test {
74+
constructor(foo: Observable<Boolean>) {}
75+
}
76+
`;
77+
assertAnnotated({
78+
message: FAILURE_STRING_NEGATED_PIPE,
79+
ruleName: ruleName,
80+
source
8681
});
82+
});
8783

88-
it('fixes un-strict equality', () => {
89-
let source = `
90-
@Component({
91-
selector: 'foobar',
92-
template: '{{ (foo | async) == false }}'
93-
~~~~~~~~~~~~~~~~~~~~~~
94-
})
95-
class Test {}
96-
`;
97-
const failures = assertAnnotated({
98-
ruleName: 'templates-no-negated-async',
99-
message: 'Async pipes must use strict equality `===` when comparing with `false`',
100-
source
101-
});
84+
it('should fail for multiple negated/unstrict equality async pipes', () => {
85+
const source = `
86+
@Component({
87+
selector: 'test',
88+
template: \`
89+
<div *ngFor="let elem of [1, 2, 3]; trackBy: trackByFn">
90+
{{ elem }}
91+
</div>
10292
103-
const res = Replacement.applyAll(source, failures[0].getFix());
104-
expect(res).to.eq(`
105-
@Component({
106-
selector: 'foobar',
107-
template: '{{ (foo | async) === false }}'
108-
~~~~~~~~~~~~~~~~~~~~~~
109-
})
110-
class Test {}
111-
`);
93+
<div *ngIf="!(foo | async)">
94+
~~~~~~~~~~~~~~
95+
{{ (foo | async) == false }}
96+
^^^^^^^^^^^^^^^^^^^^^^
97+
<div *ngIf="(foo | async) == false">
98+
#####################
99+
works!
100+
</div>
101+
</div>
102+
\`
103+
})
104+
class Test {
105+
constructor(foo: Observable<Boolean>) {}
106+
}
107+
`;
108+
assertMultipleAnnotated({
109+
failures: [
110+
{
111+
char: '~',
112+
msg: FAILURE_STRING_NEGATED_PIPE
113+
},
114+
{
115+
char: '^',
116+
msg: FAILURE_STRING_UNSTRICT_EQUALITY
117+
},
118+
{
119+
char: '#',
120+
msg: FAILURE_STRING_UNSTRICT_EQUALITY
121+
}
122+
],
123+
ruleName,
124+
source
112125
});
113126
});
114127
});
115128

116-
describe('valid expressions', () => {
117-
it('should succeed if an async pipe is not negated', () => {
118-
let source = `
129+
describe('success', () => {
130+
it('should succeed if async pipe is not negated', () => {
131+
const source = `
119132
@Component({
120-
selector: 'foobar',
133+
selector: 'test',
121134
template: '{{ (foo | async) }}'
122135
})
123136
class Test {
124-
constructor(public foo: Observable<Boolean>) {}
137+
constructor(foo: Observable<Boolean>) {}
125138
}
126139
`;
127-
assertSuccess('templates-no-negated-async', source);
140+
assertSuccess(ruleName, source);
128141
});
129142

130-
it('should succeed if an async pipe is not the last pipe in the negated chain', () => {
131-
let source = `
143+
it('should succeed if async pipe is not the last pipe in the negated chain', () => {
144+
const source = `
132145
@Component({
133-
selector: 'foobar',
134-
template: '{{ !(foo | async | someOtherFilter) }}'
146+
selector: 'test',
147+
template: '{{ !(foo | async | somethingElse) }}'
135148
})
136149
class Test {
137-
constructor(public foo: Observable<Boolean>) {}
150+
constructor(foo: Observable<Boolean>) {}
138151
}
139152
`;
140-
assertSuccess('templates-no-negated-async', source);
153+
assertSuccess(ruleName, source);
141154
});
142155

143-
it('should succeed if an async pipe uses strict equality', () => {
144-
let source = `
156+
it('should succeed if async pipe uses strict equality', () => {
157+
const source = `
145158
@Component({
146-
selector: 'foobar',
159+
selector: 'test',
147160
template: '{{ (foo | async) === false }}'
148161
})
149162
class Test {
150-
constructor(public foo: Observable<Boolean>) {}
163+
constructor(foo: Observable<Boolean>) {}
151164
}
152165
`;
153-
assertSuccess('templates-no-negated-async', source);
166+
assertSuccess(ruleName, source);
154167
});
155168

156169
it('should succeed if any other pipe is negated', () => {
157-
let source = `
170+
const source = `
158171
@Component({
159-
selector: 'foobar',
172+
selector: 'test',
160173
template: '{{ !(foo | notAnAsyncPipe) }}'
161174
})
162175
class Test {
163-
constructor(public foo: Observable<Boolean>) {}
176+
constructor(foo: Observable<Boolean>) {}
164177
}
165178
`;
166-
assertSuccess('templates-no-negated-async', source);
179+
assertSuccess(ruleName, source);
167180
});
168181
});
169182
});

0 commit comments

Comments
 (0)
Please sign in to comment.