From 2364b7cc3ad7a00f685f1018fadf160813b50000 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Tue, 21 Aug 2018 20:41:33 +0200 Subject: [PATCH] build: add tslint rule to disallow private setters (#12706) Adds a tslint rule to prevent people from adding private getters since they don't contribute to the public API and they generate more ES5 code. --- src/cdk/stepper/stepper.ts | 4 +- src/lib/autocomplete/autocomplete-trigger.ts | 4 +- src/lib/menu/menu.spec.ts | 4 +- src/lib/slider/slider.ts | 24 ++++++------ tools/package-tools/build-package.ts | 2 +- tools/tslint-rules/noPrivateGettersRule.ts | 39 ++++++++++++++++++++ tslint.json | 1 + 7 files changed, 59 insertions(+), 19 deletions(-) create mode 100644 tools/tslint-rules/noPrivateGettersRule.ts diff --git a/src/cdk/stepper/stepper.ts b/src/cdk/stepper/stepper.ts index ae892d376ab..295e17d1608 100644 --- a/src/cdk/stepper/stepper.ts +++ b/src/cdk/stepper/stepper.ts @@ -117,14 +117,14 @@ export class CdkStep implements OnChanges { /** Whether step is marked as completed. */ @Input() get completed(): boolean { - return this._customCompleted == null ? this._defaultCompleted : this._customCompleted; + return this._customCompleted == null ? this._defaultCompleted() : this._customCompleted; } set completed(value: boolean) { this._customCompleted = coerceBooleanProperty(value); } private _customCompleted: boolean | null = null; - private get _defaultCompleted() { + private _defaultCompleted() { return this.stepControl ? this.stepControl.valid && this.interacted : this.interacted; } diff --git a/src/lib/autocomplete/autocomplete-trigger.ts b/src/lib/autocomplete/autocomplete-trigger.ts index c7b01c8f07c..94bd9e8e610 100644 --- a/src/lib/autocomplete/autocomplete-trigger.ts +++ b/src/lib/autocomplete/autocomplete-trigger.ts @@ -250,7 +250,7 @@ export class MatAutocompleteTrigger implements ControlValueAccessor, OnDestroy { this.optionSelections, this.autocomplete._keyManager.tabOut.pipe(filter(() => this._overlayAttached)), this._closeKeyEventStream, - this._outsideClickStream, + this._getOutsideClickStream(), this._overlayRef ? this._overlayRef.detachments().pipe(filter(() => this._overlayAttached)) : observableOf() @@ -283,7 +283,7 @@ export class MatAutocompleteTrigger implements ControlValueAccessor, OnDestroy { } /** Stream of clicks outside of the autocomplete panel. */ - private get _outsideClickStream(): Observable { + private _getOutsideClickStream(): Observable { if (!this._document) { return observableOf(null); } diff --git a/src/lib/menu/menu.spec.ts b/src/lib/menu/menu.spec.ts index c3a10a41e98..c4bcae34397 100644 --- a/src/lib/menu/menu.spec.ts +++ b/src/lib/menu/menu.spec.ts @@ -777,7 +777,7 @@ describe('MatMenu', () => { } get overlayRect() { - return this.overlayPane.getBoundingClientRect(); + return this.getOverlayPane().getBoundingClientRect(); } get triggerRect() { @@ -788,7 +788,7 @@ describe('MatMenu', () => { return overlayContainerElement.querySelector('.mat-menu-panel'); } - private get overlayPane() { + private getOverlayPane() { return overlayContainerElement.querySelector('.cdk-overlay-pane') as HTMLElement; } } diff --git a/src/lib/slider/slider.ts b/src/lib/slider/slider.ts index ff2fd7aef4e..b2c3ee2fc86 100644 --- a/src/lib/slider/slider.ts +++ b/src/lib/slider/slider.ts @@ -348,7 +348,7 @@ export class MatSlider extends _MatSliderMixinBase get _trackBackgroundStyles(): { [key: string]: string } { const axis = this.vertical ? 'Y' : 'X'; const scale = this.vertical ? `1, ${1 - this.percent}, 1` : `${1 - this.percent}, 1, 1`; - const sign = this._invertMouseCoords ? '-' : ''; + const sign = this._shouldInvertMouseCoords() ? '-' : ''; return { // scale3d avoids some rendering issues in Chrome. See #12071. @@ -360,7 +360,7 @@ export class MatSlider extends _MatSliderMixinBase get _trackFillStyles(): { [key: string]: string } { const axis = this.vertical ? 'Y' : 'X'; const scale = this.vertical ? `1, ${this.percent}, 1` : `${this.percent}, 1, 1`; - const sign = this._invertMouseCoords ? '' : '-'; + const sign = this._shouldInvertMouseCoords() ? '' : '-'; return { // scale3d avoids some rendering issues in Chrome. See #12071. @@ -373,7 +373,7 @@ export class MatSlider extends _MatSliderMixinBase let axis = this.vertical ? 'Y' : 'X'; // For a horizontal slider in RTL languages we push the ticks container off the left edge // instead of the right edge to avoid causing a horizontal scrollbar to appear. - let sign = !this.vertical && this._direction == 'rtl' ? '' : '-'; + let sign = !this.vertical && this._getDirection() == 'rtl' ? '' : '-'; let offset = this._tickIntervalPercent / 2 * 100; return { 'transform': `translate${axis}(${sign}${offset}%)` @@ -388,8 +388,8 @@ export class MatSlider extends _MatSliderMixinBase // Depending on the direction we pushed the ticks container, push the ticks the opposite // direction to re-center them but clip off the end edge. In RTL languages we need to flip the // ticks 180 degrees so we're really cutting off the end edge abd not the start. - let sign = !this.vertical && this._direction == 'rtl' ? '-' : ''; - let rotate = !this.vertical && this._direction == 'rtl' ? ' rotate(180deg)' : ''; + let sign = !this.vertical && this._getDirection() == 'rtl' ? '-' : ''; + let rotate = !this.vertical && this._getDirection() == 'rtl' ? ' rotate(180deg)' : ''; let styles: { [key: string]: string } = { 'backgroundSize': backgroundSize, // Without translateZ ticks sometimes jitter as the slider moves on Chrome & Firefox. @@ -411,7 +411,7 @@ export class MatSlider extends _MatSliderMixinBase // For a horizontal slider in RTL languages we push the thumb container off the left edge // instead of the right edge to avoid causing a horizontal scrollbar to appear. let invertOffset = - (this._direction == 'rtl' && !this.vertical) ? !this._invertAxis : this._invertAxis; + (this._getDirection() == 'rtl' && !this.vertical) ? !this._invertAxis : this._invertAxis; let offset = (invertOffset ? this.percent : 1 - this.percent) * 100; return { 'transform': `translate${axis}(-${offset}%)` @@ -442,12 +442,12 @@ export class MatSlider extends _MatSliderMixinBase * Whether mouse events should be converted to a slider position by calculating their distance * from the right or bottom edge of the slider as opposed to the top or left. */ - private get _invertMouseCoords() { - return (this._direction == 'rtl' && !this.vertical) ? !this._invertAxis : this._invertAxis; + private _shouldInvertMouseCoords() { + return (this._getDirection() == 'rtl' && !this.vertical) ? !this._invertAxis : this._invertAxis; } /** The language direction for this slider element. */ - private get _direction() { + private _getDirection() { return (this._dir && this._dir.value == 'rtl') ? 'rtl' : 'ltr'; } @@ -597,14 +597,14 @@ export class MatSlider extends _MatSliderMixinBase // expect left to mean increment. Therefore we flip the meaning of the side arrow keys for // RTL. For inverted sliders we prefer a good a11y experience to having it "look right" for // sighted users, therefore we do not swap the meaning. - this._increment(this._direction == 'rtl' ? 1 : -1); + this._increment(this._getDirection() == 'rtl' ? 1 : -1); break; case UP_ARROW: this._increment(1); break; case RIGHT_ARROW: // See comment on LEFT_ARROW about the conditions under which we flip the meaning. - this._increment(this._direction == 'rtl' ? -1 : 1); + this._increment(this._getDirection() == 'rtl' ? -1 : 1); break; case DOWN_ARROW: this._increment(-1); @@ -646,7 +646,7 @@ export class MatSlider extends _MatSliderMixinBase // The exact value is calculated from the event and used to find the closest snap value. let percent = this._clamp((posComponent - offset) / size); - if (this._invertMouseCoords) { + if (this._shouldInvertMouseCoords()) { percent = 1 - percent; } diff --git a/tools/package-tools/build-package.ts b/tools/package-tools/build-package.ts index b32bfde3bdb..5543f575b77 100644 --- a/tools/package-tools/build-package.ts +++ b/tools/package-tools/build-package.ts @@ -39,7 +39,7 @@ export class BuildPackage { private bundler = new PackageBundler(this); /** Secondary entry-points partitioned by their build depth. */ - private get secondaryEntryPointsByDepth(): string[][] { + get secondaryEntryPointsByDepth(): string[][] { this.cacheSecondaryEntryPoints(); return this._secondaryEntryPointsByDepth; } diff --git a/tools/tslint-rules/noPrivateGettersRule.ts b/tools/tslint-rules/noPrivateGettersRule.ts new file mode 100644 index 00000000000..e9f9710c40d --- /dev/null +++ b/tools/tslint-rules/noPrivateGettersRule.ts @@ -0,0 +1,39 @@ +import * as ts from 'typescript'; +import * as Lint from 'tslint'; +import * as tsutils from 'tsutils'; + +/** + * Rule that doesn't allow private getters. + */ +export class Rule extends Lint.Rules.AbstractRule { + apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { + return this.applyWithWalker(new Walker(sourceFile, this.getOptions())); + } +} + +class Walker extends Lint.RuleWalker { + visitGetAccessor(getter: ts.GetAccessorDeclaration) { + // Check whether the getter is private. + if (!getter.modifiers || !getter.modifiers.find(m => m.kind === ts.SyntaxKind.PrivateKeyword)) { + return super.visitGetAccessor(getter); + } + + // Check that it's inside a class. + if (!getter.parent || !tsutils.isClassDeclaration(getter.parent)) { + return super.visitGetAccessor(getter); + } + + const getterName = getter.name.getText(); + const setter = getter.parent.members.find(member => { + return tsutils.isSetAccessorDeclaration(member) && member.name.getText() === getterName; + }) as ts.SetAccessorDeclaration | undefined; + + // Only log a failure if it doesn't have a corresponding setter. + if (!setter) { + this.addFailureAtNode(getter, 'Private setters generate unnecessary ' + + 'code. Use a function instead.'); + } + + return super.visitGetAccessor(getter); + } +} diff --git a/tslint.json b/tslint.json index 458b5288351..159ef70cf4d 100644 --- a/tslint.json +++ b/tslint.json @@ -98,6 +98,7 @@ "ts-loader": true, "no-exposed-todo": true, "no-import-spacing": true, + "no-private-getters": true, "setters-after-getters": true, "rxjs-imports": true, "no-host-decorator-in-concrete": [