Code tightening (prepping for Angular 8) #1212

msorens · 2019-08-11T18:43:32Z

🔩 Description: What code changed, and why?

In an effort to make the upcoming PR for the Angular 8 upgrade smaller, I undertook to clean up many of the deprecation, lint, compiler, and test errors that are due to more stringent checks. So all of the changes herein are compatible with our existing Angular/rxjs/TypeScript versions. Some of them correct errors in the code base that went unreported and, being somewhat innocuous, unnoticed.

Follow the commits if you want to see each type of change in isolation.
However, I have also added pre-review comments on the first occurrence of each category so it is easy to review from the file changes, too.

⛓️ Related Resources

👍 Definition of Done

Compiler, lint, and unit tests all still report no issues.

👟 How to Build and Test the Change

Rebuild automate-ui.
Run unit tests.
Run lint.

msorens · 2019-08-11T18:47:30Z

components/automate-ui/src/app/entities/api-tokens/api-token.effects.ts

    .pipe(
-      mergeMap(([_action, version]) =>
+      mergeMap(([_action, version]: [GetAllTokens, IAMMajorVersion]) =>


Deprecated variant of combineLatest:

combineLatest is deprecated: Pass arguments in a single array instead `combineLatest([a, b, c])`

So ostensibly the only change is adding the square brackets around all the arguments in combineLatest. In practice, however, it uncovered an issue: Each of the arguments (in this case _action and version had types inferred, but they were both assigned the type of the first argument in the array (GetAllTokens). Thus it is necessary here to enumerate the explicit types on L55 (and similarly throughout).

🤔 That's a bummer, I thought exactly these kinds of type inference improvements where coming with the bump... but wait, you're not saying that this isn't the case, you're just saying that on our current version of angular (or rather ngrx), the inference is wrong. Do you know something about the post-upgrade situation?

Yes, definitely a bummer... Unfortunately, post-upgrade has the same problem here.

msorens · 2019-08-11T18:49:20Z

components/automate-ui/src/app/entities/userperms/userperms.selectors.ts

@@ -8,7 +8,7 @@ export const userpermsState = createFeatureSelector<PermEntityState>(NGRX_REDUCE

 export const permList = createSelector(
  userpermsState,
-  (state) => map((id) => get(['byId', id], state),
+  (state) => map((id: string) => get(['byId', id], state),


Some places, like this, could not infer a type, so needed an explicit type added.

❓ Is there a way to lint for something like this?

Post-upgrade, it shows up either as a lint error or compiler error; I don't recall which.

msorens · 2019-08-11T18:51:05Z

...rc/app/page-components/server-org-filter-sidebar/server-org-filter-sidebar.component.spec.ts

@@ -30,12 +34,12 @@ describe('ServerOrgFilterSidebarComponent', () => {
  beforeEach(() => {
    fixture = TestBed.createComponent(ServerOrgFilterSidebarComponent);
    component = fixture.componentInstance;
-    this.ngrxStore = TestBed.get(Store);
+    ngrxStore = TestBed.get(Store);


An interesting set of unit test failures here. Apparently, our current code allows this on-the-fly creating of this.ngrxStore when we don't really even have a this object here in the test code(!). That does not fly, however, with the Angular 8 env, so made it an explicit local variable on L15.

msorens · 2019-08-12T03:51:06Z

...utomate-ui/src/app/page-components/sidebar-select-list/sidebar-select-list.component.spec.ts

@@ -31,7 +31,7 @@ describe('SidebarSelectListComponent', () => {
    it('interprets the label correctly', () => {
      component.label = 'Servers';

-      component.allItemsObs = Observable.create((observer: Observer<Array<string>>) => {
+      component.allItemsObs = new Observable((observer: Observer<Array<string>>) => {


Deprecated method:

create is deprecated: use new Observable() instead

This is a non-complicated change; literally just do what the error says--because our uses of create conform to the necessary pattern already.

msorens · 2019-08-12T04:02:10Z

...tomate-ui/src/app/pages/+compliance/+profile/+profile-overview/profile-overview.component.ts

+    this.sendFiles(Array.from(event.target.files)).subscribe({
+      next: fileUploads => { this.fileUploads = fileUploads; },
+      error: null,
+      complete: () => {


Deprecated variant of subscribe:

subscribe is deprecated: Use an observer instead of a complete callback

That message is misleading; subscribe is not wholly deprecated. The developers thought there were too many signatures for its own good (see ReactiveX/rxjs#4159), so instead of using this (with three arguments)...

subscribe(next, err, complete)

one needs to pass in a single argument (an "observer") like this:

subscribe({next: f1, err: f2, complete: f3})

That format also happens to make it simpler if you do not need to supply all three callbacks.

🤔 So, I think you mean error in the last code block. But from what you say, it's not required to supply something that's not need -- so, I wonder why we'd put error: null into line 217.

Yep, you're correct; that was just mechanical on my part, but don't need the error: null.

msorens · 2019-08-12T04:09:00Z

components/automate-ui/src/app/pages/project/rules/project-rules.component.ts

        takeUntil(this.isDestroyed),
-        map(([gStatus, uStatus, gpStatus]: string[]) => {
+        map(([gStatus, uStatus, gpStatus]) => {


These are not technically strings; they are of type EntityStatus which is a constrained set of strings. So I could have just done this:

map(([gStatus, uStatus, gpStatus]: EntityStatus[]) => {

But, all the arguments are the same type (see my earlier note where there were different types), so the types are correctly inferred, and there is thus no need to add an explicit type here; it would be redundant.

msorens · 2019-08-12T04:12:42Z

components/automate-ui/src/app/pages/service-groups/service-groups.component.ts

-  private currentFieldDirection: SortDirection;
-  private currentSortField: string;
+  public currentFieldDirection: SortDirection;
+  public currentSortField: string;


Interesting to note that these were used in some unit tests--even though they were private!--with no complaint in our current environment. But the upgrade will get more particular, requiring this fix.

msorens · 2019-08-12T04:16:52Z

components/automate-ui/src/app/services/http/http-client-auth.interceptor.spec.ts

@@ -36,7 +36,7 @@ describe('HttpClientAuthInterceptor', () => {

    it('when a 401 response is intercepted logs out the session', done => {
      spyOn(chefSession, 'logout');
-      httpClient.get('/endpoint').subscribe(null, done);
+      httpClient.get('/endpoint').subscribe({ error: done });


Here's a good example showing why the "observer object" is better than separate arguments ("next, error, complete" in that order). This test wants to ignore the next callback, so it gave a null. It then wants to provide an error callback, because the http call is expected to fail, and that is the jasmine done. So in the revised code, we just provide the error callback--no guesswork involved.

msorens · 2019-08-12T04:17:27Z

components/automate-ui/src/app/services/profiles/upload.service.spec.ts

-          'percent': 'percent',
-          'status': 'status',
+          'percent': 25,
+          'status': 1,
          'response': 'response'


Well, so much for strict typing... these types were plain wrong, yet our current env does not complain.

msorens · 2019-08-12T04:18:07Z

components/automate-ui/src/app/services/projects-filter/projects-filter.requests.ts


 export interface AuthorizedProjectsResponse {
-  projects: string[];
+  projects: Project[];


Whoa! Another interesting type that should never have passed muster.

srenatus

Thanks for pushing this forward. Some questions/discussion items inline 🙃

srenatus · 2019-08-12T09:21:21Z

components/automate-ui/src/app/entities/api-tokens/api-token.effects.ts

    .pipe(
-      mergeMap(([_action, version]) =>
+      mergeMap(([_action, version]: [GetAllTokens, IAMMajorVersion]) =>


🤔 That's a bummer, I thought exactly these kinds of type inference improvements where coming with the bump... but wait, you're not saying that this isn't the case, you're just saying that on our current version of angular (or rather ngrx), the inference is wrong. Do you know something about the post-upgrade situation?

srenatus · 2019-08-12T12:39:44Z

components/automate-ui/src/app/entities/roles/role.requests.ts

@@ -6,7 +6,7 @@ import { environment as env } from 'environments/environment';
 import { Role } from './role.model';

 export interface RolesResponse {
-  policies: Role[];
+  roles: Role[];


This makes me think we're using this wrong. Why did this not blow up somewhere?

srenatus · 2019-08-12T12:40:45Z

components/automate-ui/src/app/entities/userperms/userperms.selectors.ts

@@ -8,7 +8,7 @@ export const userpermsState = createFeatureSelector<PermEntityState>(NGRX_REDUCE

 export const permList = createSelector(
  userpermsState,
-  (state) => map((id) => get(['byId', id], state),
+  (state) => map((id: string) => get(['byId', id], state),


❓ Is there a way to lint for something like this?

srenatus · 2019-08-12T12:42:56Z

...tomate-ui/src/app/pages/+compliance/+profile/+profile-overview/profile-overview.component.ts

+    this.sendFiles(Array.from(event.target.files)).subscribe({
+      next: fileUploads => { this.fileUploads = fileUploads; },
+      error: null,
+      complete: () => {


🤔 So, I think you mean error in the last code block. But from what you say, it's not required to supply something that's not need -- so, I wonder why we'd put error: null into line 217.

...s/automate-ui/src/app/pages/+compliance/+scanner/containers/nodes-add/nodes-add.component.ts

srenatus · 2019-08-12T12:49:55Z

components/automate-ui/src/app/pages/client-runs/client-runs.component.ts

-    .subscribe(([nodeCounts, filterCount]) => {
+    .subscribe((values: any[]) => {
+      const nodeCounts: NodeCount = values[0];
+      const filterCount: number = values[1];


But subscribe did not take kindly when I tried the usual fix [...]

What happened? Did it throw you out? 😉

Fixes this warning from the impending Angular 8 upgrade: "create is deprecated: use new Observable() instead" Fix: Change Observable.create(... to new Observable(... Signed-off-by: michael sorens <[email protected]>

Fixes this warning from the impending Angular 8 upgrade: "subscribe is deprecated: Use an observer instead of a ... callback" Fix: Change subscribe(next, err, complete) => subscribe({next: f1, err: f2, complete: f3}) Reference: https://stackoverflow.com/a/56985787/115690 Signed-off-by: michael sorens <[email protected]>

Fixes this warning from the impending Angular 8 upgrade: "combineLatest is deprecated: Pass arguments in a single array instead `combineLatest([a, b, c])`" Fix: Change combineLatest(...) => combineLatest([...]) Signed-off-by: michael sorens <[email protected]>

Signed-off-by: michael sorens <[email protected]>

New version of Typescript is apparently smarter so found some issues, but just a few! Signed-off-by: michael sorens <[email protected]>

These all caused failures in the Angular 8 environment. Signed-off-by: michael sorens <[email protected]>

Signed-off-by: michael sorens <[email protected]>

tarablack01

@msorens Love it! LGTM 👍🏻

msorens added automate-ui chore auth-team anything that needs to be on the auth team board labels Aug 11, 2019

msorens requested review from scottopherson and a team August 11, 2019 18:43

msorens self-assigned this Aug 11, 2019

msorens commented Aug 12, 2019

View reviewed changes

msorens force-pushed the ms/angular-code-tightening branch from 7bcd135 to 8649d98 Compare August 12, 2019 04:23

srenatus approved these changes Aug 12, 2019

View reviewed changes

msorens added 7 commits August 12, 2019 09:38

Update to-be-deprecated Observable

7ca4222

Fixes this warning from the impending Angular 8 upgrade: "create is deprecated: use new Observable() instead" Fix: Change Observable.create(... to new Observable(... Signed-off-by: michael sorens <[email protected]>

combineLatest -- add necessary types

775df73

Signed-off-by: michael sorens <[email protected]>

Fix new compiler errors

2c281e9

New version of Typescript is apparently smarter so found some issues, but just a few! Signed-off-by: michael sorens <[email protected]>

Tighten up unit tests

ce5d59c

These all caused failures in the Angular 8 environment. Signed-off-by: michael sorens <[email protected]>

Review feedback

fbdbf17

Signed-off-by: michael sorens <[email protected]>

msorens force-pushed the ms/angular-code-tightening branch from 9999777 to fbdbf17 Compare August 12, 2019 16:42

tarablack01 approved these changes Aug 12, 2019

View reviewed changes

scottopherson approved these changes Aug 13, 2019

View reviewed changes

msorens merged commit 8a101d1 into master Aug 13, 2019

msorens deleted the ms/angular-code-tightening branch August 13, 2019 20:50

msorens mentioned this pull request Sep 17, 2019

Automate-ui version bump from Angular 6 -> Angular 8 #1640

Closed

msorens mentioned this pull request Sep 25, 2019

[automate-1640] Automate-ui version bump from Angular 6 -> Angu… #1707

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code tightening (prepping for Angular 8) #1212

Code tightening (prepping for Angular 8) #1212

msorens commented Aug 11, 2019 •

edited

Loading

msorens Aug 11, 2019

srenatus Aug 12, 2019

msorens Aug 12, 2019

msorens Aug 11, 2019

srenatus Aug 12, 2019

msorens Aug 12, 2019

msorens Aug 11, 2019

msorens Aug 12, 2019

msorens Aug 12, 2019

srenatus Aug 12, 2019

msorens Aug 12, 2019

msorens Aug 12, 2019

msorens Aug 12, 2019

msorens Aug 12, 2019

msorens Aug 12, 2019

msorens Aug 12, 2019

srenatus left a comment

srenatus Aug 12, 2019

srenatus Aug 12, 2019

srenatus Aug 12, 2019

srenatus Aug 12, 2019

srenatus Aug 12, 2019

srenatus Aug 12, 2019

tarablack01 left a comment

Code tightening (prepping for Angular 8) #1212

Code tightening (prepping for Angular 8) #1212

Conversation

msorens commented Aug 11, 2019 • edited Loading

🔩 Description: What code changed, and why?

⛓️ Related Resources

👍 Definition of Done

👟 How to Build and Test the Change

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

srenatus 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

tarablack01 left a comment

Choose a reason for hiding this comment

msorens commented Aug 11, 2019 •

edited

Loading