chore: integrate new lint rules and fix all issues #1488

benjamincharity · 2019-05-01T15:06:47Z

Use new shared tslint and eslint rules
- Largest lint offenses:
  - any / type coercion
  - magic numbers
  - lonely ifs
  - member access
  - mixed operators
  - html in component definition
  - whitespace
  - object shorthand
  - missing change detection
  - calling methods from templates
  - negated conditions
  - arrow format
  - readonly outputs
Update code to pass new ruleset
Update git hooks and ci to run new ruleset
Add type coercion functions for all chart types
Add type coercion functions for mouse events and keyboard events
Upgrade TypeScript version

Various learnings

ESLint doesn't fix import order
when ESLint runs TSLint commands, the fix option is not available
ESLint no-unused-imports doesn't think interfaces are used (ie OnInit)
ESLint doesn't allow a defaultSeverity setting like TSLint
ESLint allows overrides based on file globs - TSLint does not
TSLint 5.0 doesn't work when symlinked (it can't find TSLint)
ESLint object property on new line rule doesn't seem to apply to imports
ESLint to disable more than just the next line, the full comment format must be used (eg /* foo */ vs // foo)

codecov · 2019-05-01T15:07:03Z

Codecov Report

Merging #1488 into master will decrease coverage by 0.04%.
The diff coverage is 99.06%.

@@            Coverage Diff             @@
##           master    #1488      +/-   ##
==========================================
- Coverage   99.46%   99.42%   -0.05%     
==========================================
  Files         107      110       +3     
  Lines        3756     3802      +46     
  Branches      531      521      -10     
==========================================
+ Hits         3736     3780      +44     
- Misses         20       22       +2

Impacted Files	Coverage Δ
terminus-ui/input/src/input-value-accessor.ts	`100% <ø> (ø)`	⬆️
terminus-ui/chart/src/amcharts.service.ts	`100% <ø> (ø)`	⬆️
terminus-ui/form-field/src/form-field-control.ts	`100% <ø> (ø)`	⬆️
terminus-ui/icon/src/custom-icons/logo_color.ts	`100% <ø> (ø)`	⬆️
...i/loading-overlay/src/loading-overlay.component.ts	`100% <ø> (ø)`	⬆️
...ui/expansion-panel/src/accordion/accordion-base.ts	`100% <ø> (ø)`	⬆️
terminus-ui/utilities/src/version/version.ts	`100% <ø> (ø)`	⬆️
terminus-ui/icon/src/custom-icons/lightbulb.ts	`100% <ø> (ø)`	⬆️
terminus-ui/logo/src/logo-types/full-gradient.ts	`100% <ø> (ø)`	⬆️
terminus-ui/icon/src/custom-icons/logo.ts	`100% <ø> (ø)`	⬆️
... and 90 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d85b207...c183686. Read the comment docs.

shani-terminus

Review In Progress *
Next file is: terminus-ui/file-upload/src/selected-file.ts

In addition to the in-line comments, I have some general questions:

When to use which linters, which are automatic?
Will running any of the linter commands help with merging?
How do I know when 'readonly' is appropriate?

Component-general:

in autocomplete, is the makeup of OptionType type ever defined, does it change?
in CHARTS, we've changed a ChartVisualizationOption treemap to tree... will we need a deprecation notice before making this change?

shani-terminus · 2019-05-02T14:05:38Z

demo/app/components/chart/chart.component.ts

-  }
-
-  chartCreated(chart) {
+  chartCreated(chart: TsChart) {
    this.setChartData(chart, this.visualization);
  }


  // Currently using `any` here as I'm not sure how to let the consumer know what type is returned


Comment is no longer valid.

shani-terminus · 2019-05-02T14:18:27Z

demo/app/components/select/select.module.ts

@@ -13,7 +13,17 @@ import { SelectComponent } from './select.component';

 @NgModule({
  // tslint:disable-next-line:max-line-length


Comment not needed

shani-terminus · 2019-05-02T15:03:07Z

terminus-ui/button/src/button.component.ts

@@ -157,8 +154,7 @@ export class TsButtonComponent implements OnInit, OnDestroy {

    // Verify the value is allowed
    if (tsButtonFormatTypesArray.indexOf(value) < 0 && isDevMode()) {
-      console.warn(`TsButtonComponent: "${value}" is not an allowed format. ` +
-      `See TsButtonFormatTypes for available options.`);
+      console.warn(`TsButtonComponent: "${value}" is not an allowed format. See TsButtonFormatTypes for available options.`);


What are the reasons for using console.warn() vs throw.Error()?

console warn will just show a warning message in browser if that's ever got called. Depending on how consumer deals with errors that thrown, they might be bubbled up to the app, or silent out.

I've gone back and forth a bit on this.. in general I went with logs simply because they don't completely stop compilation and most of our warnings are around using a component correctly. But usually we don't need to stop all the things.

We're not 100% consistent on this so we'll need to make a more firm decision at some point.

shani-terminus · 2019-05-02T15:10:29Z

terminus-ui/button/src/button.component.spec.ts


-        it(`should set the collapseDelay to default if unset`, () => {
-          buttonComponent.format = 'collapsable';
+    describe('when format === collapsable', () => {


not a functional issue, but this is using single quotes instead of ticks.

shani-terminus · 2019-05-02T15:11:42Z

terminus-ui/button/src/button.component.spec.ts

-        expect(component['collapseWithDelay']).not.toHaveBeenCalled();
-        expect(button.classList).not.toContain('c-button--collapsed');
-      });
+    it(`should not call collapseWithDelay if the type is not collapsable`, () => {


it() vs. test().
I know this isn't a change, so maybe it just needs its own ticket. There are other occurrences throughout the file.

shani-terminus · 2019-05-02T18:24:14Z

terminus-ui/expansion-panel/testing/src/test-components.ts

@@ -1,9 +1,13 @@
 // tslint:disable: component-class-suffix
 import { CommonModule } from '@angular/common';
-import { Component, NgModule } from '@angular/core';
+import {
+  Component, NgModule,


shani-terminus · 2019-05-02T18:24:26Z

terminus-ui/expansion-panel/testing/src/test-components.ts


 import { noop } from '@terminus/ngx-tools';
-import { TS_EXPANSION_PANEL_DEFAULT_OPTIONS, TsExpansionPanelModule } from '@terminus/ui/expansion-panel';
+import {
+  TS_EXPANSION_PANEL_DEFAULT_OPTIONS, TsExpansionPanelModule,


shani-terminus · 2019-05-02T19:39:18Z

terminus-ui/file-upload/src/file-upload.component.ts

 import { TsDropProtectionService } from './drop-protection.service';
 import { TsFileImageDimensionConstraints } from './image-dimension-constraints';
-import { TS_ACCEPTED_MIME_TYPES, TsFileAcceptedMimeTypes } from './mime-types';
+import {
+  TS_ACCEPTED_MIME_TYPES, TsFileAcceptedMimeTypes,


shani-terminus · 2019-05-02T19:46:38Z

terminus-ui/file-upload/src/file-upload.component.ts

+   * @param index - The item index
+   * @return The unique ID
+   */
+  public trackByFn(index): number {


Is there a test for this?

It's being tested implicitly by other tests

shani-terminus · 2019-05-02T19:47:48Z

terminus-ui/file-upload/src/selected-file.spec.ts

+        this.onload({} as Event);
+      });
+      // eslint-disable-next-line max-len
+      public result = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAEgAAABIAQMAAABvIyEEAAAAA1BMVEXXbFn0Q9OUAAAADklEQVR4AWMYRmAUjAIAAtAAAaW+yXMAAAAASUVORK5CYII=';
    }
    // Not sure why any is needed


do we need a // tslint:disable-next-line no-any

Spec files allow any by default now.

shani-terminus

Some questions, some comments.

Most of the time multiple imports came from the same place, each had its own line, but sometimes two were together.

Weird to return undefined.

shani-terminus · 2019-05-03T13:24:25Z

terminus-ui/input/src/input.component.ts

@@ -159,6 +161,7 @@ const allowedMaskShorcuts: TsMaskShortcutOptions[] = [
 * @param item - The item to check
 * @return Whether the item is a function
 */
+// tslint:disable-next-line no-any
 function isFunction(item: any): item is Function {


ngx-tools has isFunction, can that be utilized here?

There is a note above this function about the typings not being correctly coerced when using the function from ngx-tools. There is an open ticket to explore this #1156

shani-terminus · 2019-05-03T13:27:03Z

terminus-ui/input/src/input.component.ts

@@ -938,12 +951,19 @@ export class TsInputComponent implements

    // Register this component as the associated input for the Material datepicker
    // istanbul ignore else
+    // NOTE: Dangle naming controlled by Material
+    // eslint-disable-next-line no-underscore-dangle


Since we're using no-underscore-dangle below, in addition to no-any, would it be clearer/easier to put it in once to cover both cases?

shani-terminus · 2019-05-03T13:28:42Z

terminus-ui/input/src/input.component.ts

    if (this.picker && !this.picker._datepickerInput) {
+      // NOTE: Dangle naming controlled by Material
+      /* eslint-disable no-underscore-dangle */
+      // tslint:disable-next-line no-any


I know you can combine tslint:disable's into one line, is there a way to combine tslint and eslint into one line?

Unfortunately not that I know of. Hopefully straddling two tools will be short-lived for us and we can move solely to ESLint

shani-terminus · 2019-05-03T13:31:48Z

terminus-ui/login-form/src/login-form.component.spec.ts

@@ -3,9 +3,15 @@ import {
  SimpleChange,
  SimpleChanges,
 } from '@angular/core';
-import { FormBuilder } from '@angular/forms';
+import {
+  FormBuilder, FormGroup,


shani-terminus · 2019-05-03T13:31:55Z

terminus-ui/login-form/src/login-form.component.spec.ts

 import { TsValidatorsServiceMock } from '@terminus/ui/validators/testing';

+import {
+  fakeAsync, tick,


shani-terminus · 2019-05-03T19:25:27Z

terminus-ui/tsconfig.json

@@ -4,21 +4,17 @@
    "target": "es2015",
    "rootDir": ".",
    "baseUrl": "",
+    "experimentalDecorators": true,


Does this mean we won't have any more TypeScript errors in our .specs?!

That's the hope.. I'm still getting mixed results in vim 😞

shani-terminus · 2019-05-03T19:26:27Z

terminus-ui/utilities/src/reactive-form-base/reactive-form-base.component.spec.ts

@@ -14,7 +14,7 @@ describe(`TsReactiveFormBaseComponent`, function() {
  // FIXME: Currently our coverage reporting includes spec files. This test gets us to 100%
  // coverage. We should ultimately fix our reporting so that only non-spec files count against our
  // coverage report.
-/*
+  /*
 *  describe(`METHOD_MOCK`, () => {


unneeded space

shani-terminus · 2019-05-03T19:30:15Z

terminus-ui/validation-messages/src/validation-messages.service.ts

@@ -98,6 +100,4 @@ export class TsValidationMessagesService {
    return config[validatorName];
  }



I think we're invoking this in other places. I remember recently thinking it's pretty weird to to pass a datePipe just to get a validation message

Can you clarify what you mean by this?

shani-terminus · 2019-05-03T19:35:30Z

tsconfig.json

-  "exclude": [
-    ".ng_build"
-  ]
+  }


no longer excluding .ng-build?

I believe this is outdated config

shani-terminus · 2019-05-03T19:37:24Z

tslint.json

-      "check-multiline-start"
-    ],
-    "label-position": true,
-    "max-line-length": [


These things that were removed here, are they now in eslint?

Yep. Now projects should only define their own rules for custom overrides (such as enforcing a custom prefix name for components) or places where they are knowingly diverging from agreed upon conventions.

For instance, the library needs to use console warnings so we allow that specific type through in our own rules, but a codebase like Engage should not.

atlwendy

There is a lot of work going into this!
A few comments and questions, but nothing jumps out. Looks good to me!

atlwendy · 2019-05-03T20:19:58Z

demo/app/components/paginator/paginator.component.ts

+      if (isZeroBased) {
+        this.pages = Array.from(Array(this.paginator.pagesArray.length).keys());
+      } else {
+        this.pages = Array.from(Array(this.paginator.pagesArray.length).keys()).map(v => ++v);


need to make a note here 😄

hah good call.. added

atlwendy · 2019-05-04T23:01:23Z

demo/app/components/chart/chart.component.ts

      chart.data = XY_DATA;

      const dateAxis = chart.xAxes.push(new am4charts.DateAxis());
      dateAxis.renderer.grid.template.location = 0;

      const valueAxis = chart.yAxes.push(new am4charts.ValueAxis());
-      valueAxis.tooltip.disabled = true;
+      if (valueAxis.tooltip) {


How did typescript let you set disabled attribute if it's possible to be null or undefined?

Prior to this lint PR the chart was always any so there were just no checks at all. Once I added in type coercion functions for all the types, we suddenly see I was doing something dangerous.

atlwendy · 2019-05-04T23:02:06Z

demo/app/components/navigation/navigation.component.ts

@@ -95,7 +95,7 @@ export class NavigationComponent {

  }

-  updateNav(): void {
+  public updateNav(): void {


Is public or private required now?

Yep, one of our rules now is member-access which requires it be defined for all properties. This is a 'fixable' rule so I think if you don't add anything, it'll slap public on.

We do not enforce this rule for spec or test component files

atlwendy · 2019-05-04T23:03:51Z

demo/tsconfig.app.json

  "compilerOptions": {
-    "outDir": "../out-tsc/app",
+    "outDir": "./../out-tsc/app",


dean purposely changed all those to ../ 😆

./ is part of the Angular styleguide!

atlwendy · 2019-05-04T23:04:49Z

package.json

@@ -13,7 +13,7 @@
    "url": "https://github.com/GetTerminus/terminus-ui/issues"
  },
  "scripts": {
-    "//=> Section: Demo App": "==============================",
+    "//////// Section: Demo App": "==============================",


What does this do?

Help my dumb eyes see the section better. We were getting so many scripts I was experimenting with ways to better separate sections since we can't add comments to pure JSON.

The old format simply didn't jump out enough to me so it wasn't as helpful as it could be. Trying out a new format that jumps out a bit more.

atlwendy · 2019-05-04T23:22:52Z

terminus-ui/button/src/button.component.ts

-    if (!this.interceptClick) {
-      this.clicked.emit(event);
-    } else {
+    if (this.interceptClick) {


no negative condition rules :)

Yeah, didn't realize I do this everywhere. 🤦‍♂ Especially in functions - I almost always have a catch for if no value is passed in.

atlwendy · 2019-05-04T23:30:50Z

terminus-ui/expansion-panel/src/trigger/expansion-panel-trigger.component.ts


    // Since the toggle state depends on an @Input on the panel, we need to subscribe and trigger change detection manually.
    merge(
      panel.opened, panel.closed, accordionHideToggleChange,
-      panel.inputChanges.pipe(filter((changes) => !!(changes['hideToggle'] || changes['disabled']))),
+      panel.inputChanges.pipe(filter(changes => !!(changes.hideToggle || changes.disabled))),


Does lint require you to change from [] to .?

Yep, there is a rule called dot-notation that enforces this.

The dot notation is often preferred because it is easier to read, less verbose, and works better with aggressive JavaScript minimizers. Then on top of that, for TypeScript if --noImplicitAny is turned off, property access via a string literal will be ‘any’ if the property does not exist.

This rule is disabled for spec files since we often need to use the bracket approach to access private items.

atlwendy · 2019-05-04T23:34:37Z

terminus-ui/navigation/src/navigation.component.ts

+        item.isExternal = this.isExternalLink(item.destination);
+      }
+    }
+


dean will ask you to write this block in one line.

lol.. I actually could have done better here with a map (and I'm changing) but I won't go as far as dean probably wants.. afaik the only way to do this in one line is to use the comma operator. And I personally don't think the lost clarity is worth one less line.

atlwendy · 2019-05-04T23:35:32Z

terminus-ui/paginator/src/paginator.component.spec.ts

@@ -120,7 +122,7 @@ describe(`TsPaginatorComponent`, function() {
  describe(`changePage()`, () => {

    test(`should fake a changed page event when valid`, () => {
-      spyOn(component, 'currentPageChanged').and.callThrough();
+      jest.spyOn(component, 'currentPageChanged');


what's the difference between these two? i used quite a big callThrough in the past.

and.callThrough is a Jasmine thing. When using jest.spyOn, the default behavior is to also call the original method.

atlwendy · 2019-05-04T23:36:58Z

terminus-ui/paginator/src/paginator.component.ts

-        name: name,
-        value: value,
+        name,
+        value,


benjamincharity · 2019-05-07T11:37:00Z

When to use which linters

We should always use all linters. My goal is to create composable commands in package.json. So running yarn run lint will run all applicable linters or if needed you could drill down to run a specific linter yarn run lint:eslint etc.

which are automatic?

In the lint-staged block of package.json you can see which linters are run before a commit is allowed:

  "lint-staged": {
    // Lint any changed spec files
    "terminus-ui/**/*.spec.ts": [
      "yarn run lint:tslint:spec:fix",
      "git add"
    ],

    // Lint any TypeScript files that aren't spec or mock files
    "terminus-ui/**/!(*.spec|*.mock).ts": [
      "yarn run lint:tslint:fix",
      "yarn run lint:eslint:fix",
      "git add"
    ],

    // Lint any SCSS files that changed
    "terminus-ui/**/*.scss": [
      "yarn run lint:scss",
      "git add"
    ]
  },

And most important, my IDE is set up to run yarn run lint:fix every time I save a file. This is an absolute must in my opinion.

Will running any of the linter commands help with merging?

In a way.. it should help us all use more consistent formats etc which should help some automatic merging.

How do I know when 'readonly' is appropriate?

It should be used when the item should never change. A great example is @Output. They should never be changed by anyone or internally, so they are readonly. Once we can upgrade to TypeScript 3.4 we'll be able to define readonly arrays also which will be a big help. That way we'll get a TypeScript error any time we try to mutate an array that is passed to us.

in autocomplete, is the makeup of OptionType type ever defined, does it change?

Yep, the class defines a default value for OptionType at the actual class definition:

export class TsAutocompleteComponent<OptionType = {[name: string]: any}> ...

So this works just like if we had just simply set the type to {[name: string]: any} but if a consumer ever gets a reference to our class to do something, they can overwrite our type for more strongly typed items:

type myCoolType = number;

...

@ViewChild(TsAutocompleteComponent<myCoolType>)
auto: TsAutocompleteComponent<myCoolType>;

Then for instance, when they pass in a TsAutocompleteComparatorFn they can have strong typings enforcing what type is passed to that function.

in CHARTS, we've changed a ChartVisualizationOption treemap to tree... will we need a deprecation notice before making this change?

Good catch. Normally, absolutely. In this case I rolled it into this change simply because I know that no one is even playing with charts yet.

benjamincharity · 2019-05-07T14:37:17Z

@shani-terminus @atlwendy all comments have been addressed

benjamincharity requested review from atlwendy and shani-terminus as code owners May 1, 2019 15:06

benjamincharity self-assigned this May 1, 2019

benjamincharity force-pushed the 1460-use-new-lint-configs branch from fefc013 to c6f227b Compare May 1, 2019 15:07

benjamincharity added the DO NOT MERGE PRs that are not ready to be merged. label May 1, 2019

benjamincharity force-pushed the 1460-use-new-lint-configs branch from c6f227b to 5bad3b9 Compare May 1, 2019 18:15

benjamincharity removed the DO NOT MERGE PRs that are not ready to be merged. label May 1, 2019

benjamincharity force-pushed the 1460-use-new-lint-configs branch 3 times, most recently from 748b520 to 95f772f Compare May 1, 2019 19:56

chore: integrate new lint rules and fix all issues

cc9d398

benjamincharity force-pushed the 1460-use-new-lint-configs branch from 95f772f to cc9d398 Compare May 1, 2019 20:01

shani-terminus suggested changes May 2, 2019

View reviewed changes

shani-terminus suggested changes May 3, 2019

View reviewed changes

atlwendy approved these changes May 5, 2019

View reviewed changes

WIP: PR comment cleanup

c183686

benjamincharity merged commit 8648c3d into master May 8, 2019

benjamincharity mentioned this pull request May 28, 2019

feat(Utility): expose UUID generator GetTerminus/ngx-tools#303

Merged

benjamincharity deleted the 1460-use-new-lint-configs branch June 21, 2019 20:57

		@@ -13,7 +13,17 @@ import { SelectComponent } from './select.component';

		@NgModule({
		// tslint:disable-next-line:max-line-length

		@@ -98,6 +100,4 @@ export class TsValidationMessagesService {
		return config[validatorName];
		}

chore: integrate new lint rules and fix all issues #1488

chore: integrate new lint rules and fix all issues #1488

Conversation

benjamincharity commented May 1, 2019 • edited Loading

Various learnings

codecov bot commented May 1, 2019 • edited Loading

Codecov Report

shani-terminus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shani-terminus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atlwendy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamincharity commented May 7, 2019

benjamincharity commented May 7, 2019

benjamincharity commented May 1, 2019 •

edited

Loading

codecov bot commented May 1, 2019 •

edited

Loading