Jamie/4699 frontend update deps #4835

SEAjamieD · 2021-03-17T23:26:02Z

🔩 Description: What code changed, and why?

As the dependencies we utilize and rely on within our codebase are updated and improved, its important that we continue to stay ahead of deprecations, breaking changes, and best security. We periodically examine where our dependencies are falling behind and update as necessary.

We are currently outdated in a number of packages: see image in description of #4699

after update from this branch, running npm outdated will list the following exceptions documented in README.md

Package	Current	Wanted	Latest	Location
immutable	3.8.2	3.8.2	4.0.0-rc.12	automate-ui
jasmine-core	3.6.0	3.6.0	3.7.0	automate-ui
rrule	2.4.1	2.4.1	2.6.8	automate-ui
typescript	4.0.7	4.2.3	4.2.3	automate-ui

⛓️ Related Resources

Adresses #4699

👍 Definition of Done

running ng build returns no errors
running make lint returns no errors with exception listed below
running make unit returns no errors

All automate-ui frontend dependencies are as up to date as possible with the exception of those documented in README.md.

👟 How to Build and Test the Change

option 1: - Delete your node files and perform a new installation
cd into automate-ui folder
rm -rf node_modules && npm install
please remember to run this again after switching back to whatever branch you are working on

option 2 - Save your node modules elsewhere to reuse after
cd into automate-ui folder
mv node_modules /location/to/save/your/old/node_modules
npm install

After the new install
ng build
make unit
make serve -> this will return a warning for baseHref which is being handled in #4865
All should return no errors
running npm install should report 0 vulnerabilities

It is also highly appreciated if each tester would spend some time clicking around the application, keeping an eye out for any new console errors that my expose themselves. Thankfully, almost all of the dependency updates are of a minor version so I don't expect anything much to pop up.

Note regarding TSLint Deprecation warnings

TSLint Deprecation warnings will be handled in #4700

Note for those using WallabyJS

Wallaby is currently picking up two errors in jwt.ts. It appears that these are false positives as our signin is still working as expected. I am reaching out to the Wallaby team for assistance in resolving.

✅ Checklist

I have read the CONTRIBUTING document.
Tests added/updated?
Docs added/updated?
All commits have been signed-off for the Developer Certification of Origin.

📷 Screenshots, if applicable

netlify · 2021-03-17T23:26:06Z

Deploy preview for chef-automate processing.

Building with commit ed3b4a8

https://app.netlify.com/sites/chef-automate/deploys/605c01fe6d047d000960c2c9

SEAjamieD · 2021-03-18T21:19:30Z

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

@@ -1,6 +1,6 @@
 import { Injectable } from '@angular/core';
 import { HttpErrorResponse } from '@angular/common/http';
-import { Actions, Effect, ofType } from '@ngrx/effects';
+import { Actions, createEffect, ofType } from '@ngrx/effects';


the @Effect decorator has been deprecated in favor of a more "React hooks" style of composition.
Deprecation notice: https://ngrx.io/api/effects/Effect
Usage notes for createEffect: https://ngrx.io/api/effects/createEffect#usage-notes

SEAjamieD · 2021-03-18T21:20:02Z

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

+  getAllTokens$ = createEffect(() => {
+    return this.actions$.pipe(ofType<GetAllTokens>(ApiTokenActionTypes.GET_ALL),
    mergeMap((_action) =>
      this.requests.getAll().pipe(
        map(resp => new GetAllTokensSuccess(resp)),
        catchError((error: HttpErrorResponse) => of(new GetAllTokensFailure(error))))
    ));
+  });


You'll see this new format used throughout all our effects files moving forward and its how we should continue to compose effects.

SEAjamieD · 2021-03-19T19:35:46Z

components/automate-ui/src/app/entities/service-groups/service-groups.requests.ts

@@ -151,7 +151,7 @@ export class ServiceGroupsRequests {
    }, searchParam, filters);
  }

-  public deleteServicesById(listOfIds: number[]): any {
+  public deleteServicesById(listOfIds: number[]): Observable<any> {


Thanks to @msorens for helping realize the error in this related effect was coming from the lack of an Observable signature. The should be updated to be more specific as well

Looking into this more, I'm going to investigate the proper type in a new branch so as not to hold up these dependencies.

SEAjamieD · 2021-03-19T20:46:21Z

...ents/automate-ui/src/app/modules/infra-proxy/client-details/client-details.component.spec.ts

@@ -32,6 +32,7 @@ describe('ClientDetailsComponent', () => {
        MockComponent({ selector: 'chef-tr' }),
        MockComponent({ selector: 'chef-th' }),
        MockComponent({ selector: 'chef-td' }),
+        MockComponent({ selector: 'chef-snippet', inputs: ['code'] }),


Some cleanup of missing mock-selectors. I'm actually surprised these made it through bulidkite.

SEAjamieD · 2021-03-19T20:47:13Z

...ents/automate-ui/src/app/modules/infra-proxy/data-bags-list/data-bags-list.component.spec.ts

@@ -46,7 +47,8 @@ describe('DataBagsListComponent', () => {
        ReactiveFormsModule,
        RouterTestingModule,
        StoreModule.forRoot(ngrxReducers, { runtimeChecks })
-      ]
+      ],
+      schemas: [CUSTOM_ELEMENTS_SCHEMA]


Several test files were missing the CUSTOM_ELEMENTS_SCHEMA required for our custom components and as a result were throwing linting errors.

msorens

Nice work!

Should add to your "done" criteria the fact that npm install should report "found 0 vulnerabilities" -- that is not always the case and should be celebrated!!
Should mention in the PR preamble that wallaby is reporting 2 errors and if there are plans to resolve those.
Should mention that the TSLint deprecation warnings will be dealt with in a separate issue.
Should also mention that make serve should display no errors or warnings. Currently, it does have one (I would add this as a new issue to the Bringing dependencies up-to-date epic.)

Option "baseHref" is deprecated: Use the "baseHref" option in the browser builder instead.

msorens · 2021-03-21T18:11:33Z

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

+  latestOptions$ = createEffect(() =>
+    observableInterval(1000 * this.POLLING_INTERVAL_IN_SECONDS).pipe(
+    mergeMap(this.loadOptionsAction$())));

  // Fast-initialize project filter just from local storage
-  @Effect()
-  initOptions$ = this.actions$.pipe(
+  initOptions$ = createEffect(() => {
+    return this.actions$.pipe(


Let's be consistent: as you have it here, there are two equivalent syntaxes for writing the createEffect expression. I want to urge the more concise format (L33). No need to introduce a separate block of code (L38-39) when there is just a single return. (Apologies that that does make a lot of editing for you. 😁 )

msorens · 2021-03-21T18:16:02Z

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

    ofType<SaveOptions>(ProjectsFilterActionTypes.SAVE_OPTIONS),
    tap(({ payload }) => this.projectsFilter.storeOptions(payload))
-  );
+    ), { dispatch: false });


Why was this dispatch added? What does it do?

We need to use {dispatch: false} whenever our effect isn't returning an action so that we don't cause an infinite loop.

Non-dispatching Effects
Sometimes you don't want effects to dispatch an action, for example when you only want to log or navigate based on an incoming action. But when an effect does not dispatch another action, the browser will crash because the effect is both 'subscribing' to and 'dispatching' the exact same action, causing an infinite loop. To prevent this, add { dispatch: false } to the createEffect function as the second argument.

In this case, dispatch wasn't added here, it already existed but it now looks a little different in the new style of writing effects.

In the old format it was composed as

@Effect( {dispatch: false} )

in the new style

createEffect( () => some code, {dispatch: false})

msorens · 2021-03-21T18:18:47Z

components/automate-ui/README.md

@@ -55,17 +55,9 @@ Certain packages in package.json are constrained for the reasons detailed here.
 At any future moment though, the reasons for constraint here could be invalidated, so this should be updated as needed when package.json is updated.
 While it is problematic to document this information due to the maintenance burden, the value of having this in one place outweighs that burden.

-### Packages @ngrx/* =10.1.0
+### Package typescript: ^4.0.7


Nice--thanks for keeping these docs current!

msorens · 2021-03-21T18:19:51Z

components/automate-ui/src/app/helpers/jwt/jwt.ts

@@ -57,7 +57,7 @@ export class Jwt {
  static parseLicenseToken(token: string): LicenseToken | null {
    let tok: LicenseToken;
    try {
-      tok = jwtDecode(token);
+      tok = jwt_decode(token);


Is this change still causing errors to be reported by Wallaby?

yeah it is, I meant to send them a note last week but didn't. I'll get one out to them today.

The wallaby errors have since disappeared 🤷

msorens · 2021-03-24T03:02:19Z

components/automate-ui/src/app/entities/clients/client.effects.ts


-  @Effect()
-  resetKeyClient$ = this.actions$.pipe(
+  resetKeyClient$ = createEffect(() => this.actions$.pipe(


[nitpick] Of the 364 effects, there are just 14 (like this one) that do not have a line break after the =>. 😉