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

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

if you insist

I don't 😇

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.

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 :-)

Ooops 😞

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

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

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.

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

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.

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.

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

+1

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

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