style(aio): enforce strict TypeScript checks #21342

petebacondarwin · 2018-01-05T23:01:30Z

Unfortunately the version of @angular/service-worker that we are using (^1.0.0-beta.16)
contains an implicit any which is the only thing preventing the enforcement of noImplicitAny.
This looks like it is fixed in v5+ of the package.

Closes #20646

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

mary-poppins · 2018-01-05T23:08:09Z

You can preview 78cebb9 at https://pr21342-78cebb9.ngbuilds.io/.

gkalpak

Overall LGTM 👍
Just a couple of nits and a linting error.

gkalpak · 2018-01-08T20:32:41Z

aio/src/app/app.component.spec.ts

@@ -1213,9 +1213,9 @@ class TestHttpClient {
      data = this.navJson;
    } else {
      const match = /generated\/docs\/(.+)\.json/.exec(url);
-      const id = match[1];
+      const id = match![1]!;


LoL
(Maybe move the first ! to the line above?)

if you insist

I don't 😇

gkalpak · 2018-01-08T21:09:35Z

aio/src/app/sw-updates/sw-updates.service.spec.ts

@@ -31,7 +31,7 @@ describe('SwUpdatesService', () => {
    checkInterval = (service as any).checkInterval;
  };
  const tearDown = () => service.ngOnDestroy();
-  const run = specFn => () => {
+  const run = (specFn: Function) => () => {


How come tslint doesn't complain about Function. It usually prefers a specific function signature (e.g. () => void) 😃

No idea but I changed it to VoidFunction just to keep you happy.

gkalpak · 2018-01-09T11:12:36Z

aio/src/app/layout/doc-viewer/doc-viewer.component.ts

@@ -193,15 +193,15 @@ export class DocViewerComponent implements DoCheck, OnDestroy {

          elem.style.transition = '';
          return animationsDisabled
-              ? this.void$.do(() => elem.style[prop] = to)
+              ? this.void$.do(() => (elem.style as any)[prop] = to)


You can avoid the as any (here and below), by giving prop a type of keyof CSSStyleDeclaration (which also improves type-safety for the animateProp() function 🎉).

not quite so simple since length and parentRule properties are readonly :-)

Arguably, this points out a bug in the code :-) I have put a check for those two properties.

gkalpak · 2018-01-09T11:18:42Z

aio/src/app/shared/custom-icon-registry.spec.ts

@@ -11,7 +11,7 @@ describe('CustomIconRegistry', () => {
      { name: 'test_icon', svgSource: svgSrc }
    ];
    const registry = new CustomIconRegistry(mockHttp, mockSanitizer, svgIcons);
-    let svgElement: SVGElement;
+    let svgElement: any;


Why is this needed? Doesn't getNamedSvgIcon return an observable of SVGElements?
Is it for comparing with the result of createSvg()? Wouldn't it be more appropriate to "fix" createSvg() instead:

function createSvg(svgSrc) { const div = document.createElement('div'); div.innerHTML = svgSrc; - return div.querySelector('svg'); + return div.querySelector<SVGElement>('svg')!; }

+1
I fixed by relaxing the return type of createSvg(svgSrc: string): SVGElement

gkalpak · 2018-01-09T11:29:35Z

aio/src/app/shared/location.service.ts

    });

    swUpdates.updateActivated.subscribe(() => this.swUpdateActivated = true);
  }

  // TODO?: ignore if url-without-hash-or-search matches current location?
-  go(url: string) {
+  go(url: string|null|undefined) {


Wouldn't it be preferrable to have go(url?: string|null)? Do we need to explicitly pass undefined?

So we do have explicit tests for each of these scenarios: https://github.com/angular/angular/blob/master/aio/src/app/shared/location.service.spec.ts#L272
But looking at the code, we never call with undefined or null so actually let's make it tighter.

gkalpak · 2018-01-09T11:31:59Z

aio/src/app/shared/location.service.ts

-  search(): { [index: string]: string; } {
-    const search = {};
+  search() {
+    const search: { [index: string]: string|undefined; } = {};


Why do we need the |undefined part?

~~I don't think we do 😱 - perhaps I was being over-zealous!~~

Actually we have a test for this very scenario: https://github.com/angular/angular/blob/master/aio/src/app/shared/location.service.spec.ts#L339

gkalpak · 2018-01-09T11:37:51Z

aio/src/app/shared/location.service.ts

@@ -80,7 +80,7 @@ export class LocationService {
    return search;
  }

-  setSearch(label: string, params: {}) {
+  setSearch(label: string, params: { [key: string]: string|undefined}) {


AFAICT, the value type should be string|null|undefined.

OK, so as far as we use this we never pass null, so instead of relaxing the params type I am going to fix the use of double equals below this.

gkalpak · 2018-01-09T11:49:08Z

aio/src/app/shared/scroll-spy.service.spec.ts

@@ -234,7 +235,7 @@ describe('ScrollSpyService', () => {

    it('should remember and emit the last active item to new subscribers', () => {
      const items = [{index: 1}, {index: 2}, {index: 3}] as ScrollItem[];
-      let lastActiveItem: ScrollItem | null;
+      let lastActiveItem: ScrollItem|null|undefined;


Nit:
|undefined is somewhat confusing imo (and unnecessarily less strict - which is not a concern is tests of course). If it is just for avoiding the "used before being assigned" error, it might be more clear to initialize the value (e.g. let lastActiveItem: ScrollItem|null = null) or use ! when using it (e.g. expect(lastActiveItem!))?

(I understand this stems from TypeScript's current limitation to infer that a value might actual be set (through a synchronous function call) and falls under the "Super Nit" category, so feel free to ignore 😁)

OK, so initially I was concerned that we actually check that this value is null at the end of the test, so if none of the subscription handlers were called then this pass despite not being correct. But I now notice that before we check for it becoming null we check that it is some non-null value, so if none of the handler ran then the first expectation would fail before we get the false positive.

gkalpak · 2018-01-09T11:53:30Z

aio/src/app/shared/scroll.service.spec.ts

@@ -111,7 +111,7 @@ describe('ScrollService', () => {

      const topOfPage = new MockElement();
      document.getElementById.and
-              .callFake(id  => id === 'top-of-page' ? topOfPage : null);
+              .callFake((id:string)  => id === 'top-of-page' ? topOfPage : null);


Missing space before string. (While you are at it, you can remove the double-space before => 😁)

gkalpak · 2018-01-09T12:02:27Z

aio/src/app/search/interfaces.ts

@@ -5,7 +5,7 @@ export interface SearchResults {

 export interface SearchResult {
  path: string;
-  title: string;
+  title?: string;


OOC, under what circumstances can a search result have no associated title?

Never :-) Or at least only if the data is bad.

IgorMinar · 2018-01-10T06:54:16Z

If I understand Pete's comment correctly, this is blocked on #19795. right?

petebacondarwin · 2018-01-10T10:44:13Z

@IgorMinar : No, this is not blocked on #19795 but we just can't turn on the final noImplicitAny rule until that lands. Our code is all now compliant with that rule, we just don't have it turned on.

mary-poppins · 2018-01-10T10:50:09Z

You can preview d1aa41e at https://pr21342-d1aa41e.ngbuilds.io/.

mary-poppins · 2018-01-10T10:52:12Z

You can preview 40a4e68 at https://pr21342-40a4e68.ngbuilds.io/.

mary-poppins · 2018-01-10T15:03:56Z

You can preview 124c34e at https://pr21342-124c34e.ngbuilds.io/.

IgorMinar · 2018-01-10T16:59:43Z

@petebacondarwin I see. that makes sense. thanks

gkalpak

LGTM (as soon as CI is green 😃)
🎉

mary-poppins · 2018-01-11T14:38:26Z

You can preview d133c0e at https://pr21342-d133c0e.ngbuilds.io/.

petebacondarwin · 2018-01-11T14:50:22Z

oh I forgot about the e2e scripts

Closes angular#20646

mary-poppins · 2018-01-11T18:54:53Z

You can preview 537e4d0 at https://pr21342-537e4d0.ngbuilds.io/.

gkalpak · 2018-01-11T22:28:55Z

File sizes were affected for some reason 😁
The limits need to be updated.

Closes #20646 PR Close #21342

Closes angular#20646 PR Close angular#21342

angular-automatic-lock-bot · 2019-09-13T04:50:55Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

petebacondarwin added comp: aio action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Jan 5, 2018

petebacondarwin requested a review from gkalpak January 5, 2018 23:01

googlebot added the cla: yes label Jan 5, 2018

petebacondarwin added this to REVIEW in docs-infra Jan 5, 2018

gkalpak requested changes Jan 9, 2018

View reviewed changes

petebacondarwin force-pushed the aio-strict branch from d1aa41e to 40a4e68 Compare January 10, 2018 10:45

petebacondarwin force-pushed the aio-strict branch from 40a4e68 to 124c34e Compare January 10, 2018 14:55

gkalpak approved these changes Jan 11, 2018

View reviewed changes

gkalpak added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 11, 2018

petebacondarwin force-pushed the aio-strict branch from 124c34e to d133c0e Compare January 11, 2018 14:29

style(aio): enforce strict TypeScript checks

537e4d0

Closes angular#20646

petebacondarwin force-pushed the aio-strict branch from d133c0e to 537e4d0 Compare January 11, 2018 18:46

petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jan 12, 2018

petebacondarwin moved this from REVIEW to MERGE in docs-infra Jan 12, 2018

petebacondarwin removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Jan 12, 2018

alexeagle pushed a commit that referenced this pull request Jan 12, 2018

style(aio): enforce strict TypeScript checks (#21342)

71dd92b

Closes #20646 PR Close #21342

alexeagle closed this in c5c6d84 Jan 12, 2018

gkalpak removed this from MERGE in docs-infra Jan 15, 2018

petebacondarwin deleted the aio-strict branch January 16, 2018 11:04

leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018

style(aio): enforce strict TypeScript checks (angular#21342)

168abae

Closes angular#20646 PR Close angular#21342

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

style(aio): enforce strict TypeScript checks #21342

style(aio): enforce strict TypeScript checks #21342

Conversation

petebacondarwin commented Jan 5, 2018 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

mary-poppins commented Jan 5, 2018

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Jan 9, 2018 • edited

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

petebacondarwin Jan 10, 2018 • edited

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

petebacondarwin Jan 10, 2018 • edited

Choose a reason for hiding this comment

IgorMinar commented Jan 10, 2018

petebacondarwin commented Jan 10, 2018

mary-poppins commented Jan 10, 2018

mary-poppins commented Jan 10, 2018

mary-poppins commented Jan 10, 2018

IgorMinar commented Jan 10, 2018

gkalpak left a comment

Choose a reason for hiding this comment

mary-poppins commented Jan 11, 2018

petebacondarwin commented Jan 11, 2018

mary-poppins commented Jan 11, 2018

gkalpak commented Jan 11, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

petebacondarwin commented Jan 5, 2018 •

edited

gkalpak Jan 9, 2018 •

edited

petebacondarwin Jan 10, 2018 •

edited

petebacondarwin Jan 10, 2018 •

edited