Skip to content

refactor(rule): rename/clean up 'component-change-detection' #782

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 3, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 0 additions & 79 deletions src/componentChangeDetectionRule.ts

This file was deleted.

2 changes: 1 addition & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
export { Rule as AngularWhitespaceRule } from './angularWhitespaceRule';
export { Rule as ComponentChangeDetectionRule } from './componentChangeDetectionRule';
export { Rule as ComponentClassSuffixRule } from './componentClassSuffixRule';
export { Rule as ComponentSelectorRule } from './componentSelectorRule';
export { Rule as ContextualDecoratorRule } from './contextualDecoratorRule';
Expand All @@ -25,6 +24,7 @@ export { Rule as NoQueriesMetadataPropertyRule } from './noQueriesMetadataProper
export { Rule as NoUnusedCssRule } from './noUnusedCssRule';
export { Rule as PipePrefixRule } from './pipePrefixRule';
export { Rule as PreferInlineDecoratorRule } from './preferInlineDecoratorRule';
export { Rule as PreferOnPushComponentChangeDetectionRule } from './preferOnPushComponentChangeDetectionRule';
export { Rule as PreferOutputReadonlyRule } from './preferOutputReadonlyRule';
export { Rule as RelativeUrlPrefixRule } from './relativeUrlPrefixRule';
export { Rule as TemplateAccessibilityAltTextRule } from './templateAccessibilityAltTextRule';
Expand Down
54 changes: 54 additions & 0 deletions src/preferOnPushComponentChangeDetectionRule.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import { IRuleMetadata, RuleFailure } from 'tslint/lib';
import { AbstractRule } from 'tslint/lib/rules';
import { dedent } from 'tslint/lib/utils';
import { isPropertyAccessExpression, SourceFile } from 'typescript/lib/typescript';
import { ComponentMetadata } from './angular/metadata';
import { NgWalker } from './angular/ngWalker';
import { getDecoratorPropertyInitializer } from './util/utils';

const ON_PUSH = 'OnPush';

export class Rule extends AbstractRule {
static readonly metadata: IRuleMetadata = {
description: `Enforces component's change detection to ChangeDetectionStrategy.${ON_PUSH}.`,
options: null,
optionsDescription: 'Not configurable.',
rationale: dedent`
By default Angular uses the ChangeDetectionStrategy.Default.

This strategy doesn’t assume anything about the application, therefore every time something changes in our application, as a result of various user events, timers, XHR, promises, etc., a change detection will run on all components.

By using ChangeDetectionStrategy.${ON_PUSH}, Angular will only run the change detection cycle in the following cases:
* Inputs references change.
* An event originated from the component or one of its children.
* If manually called.
`,
ruleName: 'prefer-on-push-component-change-detection',
type: 'functionality',
typescriptOnly: true
};

static readonly FAILURE_STRING = `The changeDetection value of a component should be set to ChangeDetectionStrategy.${ON_PUSH}`;

apply(sourceFile: SourceFile): RuleFailure[] {
return this.applyWithWalker(new PreferOnPushComponentChangeDetectionWalker(sourceFile, this.getOptions()));
}
}

class PreferOnPushComponentChangeDetectionWalker extends NgWalker {
protected visitNgComponent(metadata: ComponentMetadata): void {
this.validateComponent(metadata);
super.visitNgComponent(metadata);
}

private validateComponent(metadata: ComponentMetadata): void {
const { decorator: metadataDecorator } = metadata;
const changeDetectionExpression = getDecoratorPropertyInitializer(metadataDecorator, 'changeDetection');

if (!changeDetectionExpression) {
this.addFailureAtNode(metadataDecorator, Rule.FAILURE_STRING);
} else if (isPropertyAccessExpression(changeDetectionExpression) && changeDetectionExpression.name.text !== ON_PUSH) {
this.addFailureAtNode(changeDetectionExpression, Rule.FAILURE_STRING);
}
}
}
14 changes: 8 additions & 6 deletions src/useComponentViewEncapsulationRule.ts
Original file line number Diff line number Diff line change
@@ -1,21 +1,23 @@
import { IRuleMetadata, RuleFailure, Rules } from 'tslint/lib';
import { IRuleMetadata, RuleFailure } from 'tslint/lib';
import { AbstractRule } from 'tslint/lib/rules';
import { isPropertyAccessExpression, SourceFile } from 'typescript/lib/typescript';
import { ComponentMetadata } from './angular/metadata';
import { NgWalker } from './angular/ngWalker';
import { getDecoratorPropertyInitializer } from './util/utils';

const NONE = 'None';

export class Rule extends AbstractRule {
static readonly metadata: IRuleMetadata = {
description: 'Disallows using of ViewEncapsulation.None.',
description: `Disallows using ViewEncapsulation.${NONE}.`,
options: null,
optionsDescription: 'Not configurable.',
ruleName: 'use-component-view-encapsulation',
type: 'maintainability',
typescriptOnly: true
};

static readonly FAILURE_STRING = 'Using "ViewEncapsulation.None" makes your styles global, which may have an unintended effect';
static readonly FAILURE_STRING = `Using ViewEncapsulation.${NONE} makes your styles global, which may have an unintended effect`;

apply(sourceFile: SourceFile): RuleFailure[] {
return this.applyWithWalker(new ViewEncapsulationWalker(sourceFile, this.getOptions()));
Expand All @@ -29,12 +31,12 @@ class ViewEncapsulationWalker extends NgWalker {
}

private validateComponent(metadata: ComponentMetadata): void {
const encapsulation = getDecoratorPropertyInitializer(metadata.decorator, 'encapsulation');
const encapsulationExpression = getDecoratorPropertyInitializer(metadata.decorator, 'encapsulation');

if (!encapsulation || (isPropertyAccessExpression(encapsulation) && encapsulation.name.text !== 'None')) {
if (!encapsulationExpression || (isPropertyAccessExpression(encapsulationExpression) && encapsulationExpression.name.text !== NONE)) {
return;
}

this.addFailureAtNode(encapsulation, Rule.FAILURE_STRING);
this.addFailureAtNode(encapsulationExpression, Rule.FAILURE_STRING);
}
}
15 changes: 9 additions & 6 deletions src/util/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,13 +105,16 @@ export const getDecoratorArgument = (decorator: Decorator): ObjectLiteralExpress
};

export const getDecoratorPropertyInitializer = (decorator: Decorator, name: string): Expression | undefined => {
const args = isCallExpression(decorator.expression) ? decorator.expression.arguments[0] : undefined;
const properties = createNodeArray(args && isObjectLiteralExpression(args) ? args.properties : undefined);
const args = getDecoratorArgument(decorator);

return properties
.filter(prop => prop.name && isIdentifier(prop.name) && prop.name.text === name)
.map(prop => (isPropertyAssignment(prop) ? prop.initializer : undefined))
.pop();
if (!args || !isObjectLiteralExpression(args)) return undefined;

const properties = createNodeArray(args.properties);
const property = properties.find(prop => !!(prop.name && prop.name.getText() === name));

if (!property || !isPropertyAssignment(property)) return undefined;

return property.initializer;
};

export const getDecoratorName = (decorator: Decorator): string | undefined =>
Expand Down
47 changes: 0 additions & 47 deletions test/componentChangeDetectionRule.spec.ts

This file was deleted.

54 changes: 54 additions & 0 deletions test/preferOnPushComponentChangeDetectionRule.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import { Rule } from '../src/preferOnPushComponentChangeDetectionRule';
import { assertAnnotated, assertSuccess } from './testHelper';

const {
FAILURE_STRING,
metadata: { ruleName }
} = Rule;

describe(ruleName, () => {
describe('failure', () => {
it('should fail if ChangeDetectionStrategy.Default is set', () => {
const source = `
@Component({
changeDetection: ChangeDetectionStrategy.Default
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
})
class Test {}
`;
assertAnnotated({
message: FAILURE_STRING,
ruleName,
source
});
});

it('should fail if no ChangeDetectionStrategy is explicitly set', () => {
const source = `
@Component({
~~~~~~~~~~~~
selector: 'foo'
})
~~
class Test {}
`;
assertAnnotated({
message: FAILURE_STRING,
ruleName,
source
});
});
});

describe('success', () => {
it('should succeed if ChangeDetectionStrategy.OnPush is set', () => {
const source = `
@Component({
changeDetection: ChangeDetectionStrategy.OnPush
})
class Test {}
`;
assertSuccess(ruleName, source);
});
});
});