Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upgrade helix-front to Angular 7.2 #2084

Closed
micahstubbs opened this issue May 8, 2022 · 36 comments · Fixed by #2087
Closed

upgrade helix-front to Angular 7.2 #2084

micahstubbs opened this issue May 8, 2022 · 36 comments · Fixed by #2087

Comments

@micahstubbs
Copy link
Contributor

https://update.angular.io/?l=3&v=6.1-7.2

see #2053 for motivation, details, and history.

@micahstubbs
Copy link
Contributor Author

micahstubbs commented May 8, 2022

Support for using the ngModel input property and ngModelChange event with reactive form directives has been deprecated in v6 and removed in v7.

It looks like we do use ngModel:

Screen Shot 2022-05-08 at 9 52 47 AM

Let's do a search and see what the recommended replacement is.

@micahstubbs
Copy link
Contributor Author

@micahstubbs
Copy link
Contributor Author

@micahstubbs
Copy link
Contributor Author

@micahstubbs
Copy link
Contributor Author

encountered this problem https://stackoverflow.com/questions/39152071/cant-bind-to-formgroup-since-it-isnt-a-known-property-of-form

and fixed by importing ReactiveFormsModule from @angular/forms in our SharedModule.

@micahstubbs
Copy link
Contributor Author

It looks like I am unable to test this form change, since I do not have a Helix backend configured. Let's see if there is an alternate way to test this change 🤔

http://localhost:4200/helix.default

Screen Shot 2022-05-10 at 1 07 04 PM

@micahstubbs
Copy link
Contributor Author

npm run test
# TOTAL: 74 SUCCESS

passes, so that is a good sign.

@micahstubbs
Copy link
Contributor Author

ah hah! the login dialog uses the input dialog component. This is a way to manually test this change from the UI without a Helix backend configured.

Screen Shot 2022-05-10 at 1 09 01 PM

@micahstubbs
Copy link
Contributor Author

micahstubbs commented May 10, 2022

for reference, this is what the form should look like:

Screen Shot 2022-05-10 at 2 42 06 PM

@micahstubbs
Copy link
Contributor Author

Let's try to upgrade to 7.2 first and then try the form again. Maybe the 7.2 error will be helpful..

@micahstubbs
Copy link
Contributor Author

micahstubbs commented May 10, 2022

There might be breaking changes that affect helix-front in TypeScript 3.1:
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-1.html

@micahstubbs
Copy link
Contributor Author

micahstubbs commented May 10, 2022

Here are the versions we need for the major libraries:

Angular CLI version Angular version Node.js version TypeScript version RxJS version
~7.3.9 ~7.2.15 ^8.9.4 || ^10.9.0 ~3.2.4 ^6.3.3

https://gist.github.com/LayZeeDK/c822cc812f75bb07b7c55d07ba2719b3

@micahstubbs
Copy link
Contributor Author

Ah, we see that same Angular dep parsing bug again. Lets's remove the other deps like before to work around this bug.

 NG_DISABLE_VERSION_CHECK=1 npx @angular/cli@7 update @angular/cli@7 @angular/core@7

To disable this warning use "ng config -g cli.warnings.versionMismatch false".
Cannot read property 'split' of null

@micahstubbs
Copy link
Contributor Author

Let's try removing these packages temporarily:

"angulartics2": "^2.2.1",
"body-parser": "^1.17.2",
"core-js": "^2.4.1",
"d3-shape": "^1.2.0",
"dotenv": "^4.0.0",
"express": "^4.15.3",
"express-session": "^1.15.6",
"hammerjs": "^2.0.8",
"ldapjs": "^1.0.2",
"lodash": "^4.17.12",
"moment": "^2.29.2",
"morgan": "^1.8.2",
"ngx-clipboard": "^11.1.5",
"ngx-json-viewer": "^2.3.0",
"ngx-vis": "^0.1.0",
"node-sass": "4.5.3",
"request": "^2.81.0",
"vis": "^4.21.0",
"@swimlane/ngx-charts": "^8.0.0",
"@swimlane/ngx-datatable": "^13.0.0",
"@swimlane/ngx-graph": "6.0.0",

based on this old comment: #2053 (comment)

@micahstubbs
Copy link
Contributor Author

micahstubbs commented May 10, 2022

Now we see:

NG_DISABLE_VERSION_CHECK=1 npx @angular/cli@7 update @angular/cli@7 @angular/core@7
./Release/.deps/Release/obj.target/fse/fsevents.o.d.raw 

Your global Angular CLI version (7.3.10) is greater than your local
version (6.2.9). The local Angular CLI version is used.

To disable this warning use "ng config -g cli.warnings.versionMismatch false".
                  
Package "@angular/compiler-cli" has an incompatible peer dependency to
"typescript" (requires ">=3.1.1 <3.3", would install "3.9.10")
                  
Package "@angular/platform-server" has a missing peer dependency of
"@angular/http" @ "7.2.16".

Incompatible peer dependencies found. See above.

@micahstubbs
Copy link
Contributor Author

micahstubbs commented May 10, 2022

Hooray, it looks like that succeeded. 🎉

We are rewarded with this todo list of missing peer dependency warnings:

@angular/cdk@6.1.0 requires @angular/core@>=6.0.0-beta.0 <7.0.0
@angular/cdk@6.1.0 requires @angular/common@>=6.0.0-beta.0 <7.0.0
@angular/flex-layout@6.0.0-beta.18 requires @angular/core@>=6.0.0 <7.0.0
@angular/flex-layout@6.0.0-beta.18 requires @angular/common@>=6.0.0 <7.0.0
@angular/material@6.1.0 requires @angular/core@>=6.0.0-beta.0 <7.0.0
@angular/material@6.1.0 requires @angular/common@>=6.0.0-beta.0 <7.0.0
@ngtools/webpack@6.2.9 requires typescript@~2.4.0 || ~2.5.0 || ~2.6.0 || ~2.7.0 || ~2.8.0 || ~2.9.0
tsickle@0.32.1 requires typescript@>=2.4.2 <2.10

full log: https://gist.github.com/micahstubbs/3c28537ef245fe664f4572e7874f858d

@micahstubbs
Copy link
Contributor Author

micahstubbs commented May 10, 2022

Let's try @angular/flex-layout@7.0.0-beta.24

npm i @angular/flex-layout@7.0.0-beta.24

https://github.com/angular/flex-layout/tags?after=9.0.0-beta.29

angular/flex-layout@b2e2064

Looks like that worked. No complaints from npm on install.

@micahstubbs
Copy link
Contributor Author

micahstubbs commented May 10, 2022

Next, let's update tsickle.

Here's the commit we want git blame

npm i tsickle@0.34.2

@micahstubbs
Copy link
Contributor Author

Great, no more peer dep warnings for now. Let's add back those packages we temporarily removed to work around Angular's version parsing bug:

"angulartics2": "^2.2.1",
"body-parser": "^1.17.2",
"core-js": "^2.4.1",
"d3-shape": "^1.2.0",
"dotenv": "^4.0.0",
"express": "^4.15.3",
"express-session": "^1.15.6",
"hammerjs": "^2.0.8",
"ldapjs": "^1.0.2",
"lodash": "^4.17.12",
"moment": "^2.29.2",
"morgan": "^1.8.2",
"ngx-clipboard": "^11.1.5",
"ngx-json-viewer": "^2.3.0",
"ngx-vis": "^0.1.0",
"node-sass": "4.5.3",
"request": "^2.81.0",
"vis": "^4.21.0",
"@swimlane/ngx-charts": "^8.0.0",
"@swimlane/ngx-datatable": "^13.0.0",
"@swimlane/ngx-graph": "6.0.0",

@micahstubbs
Copy link
Contributor Author

Now, we npm i again.

First off, we notice several deprecated package warnings:

npm WARN deprecated vis@4.21.0: Please consider using https://github.com/visjs
npm WARN deprecated request@2.88.2: request has been deprecated, see https://github.com/request/request/issues/3142
npm WARN deprecated har-validator@5.1.5: this library is no longer supported
npm WARN deprecated uuid@3.4.0: Please upgrade  to version 7 or higher.  Older versions may use Math.random() in certain circumstances, which is known to be problematic.  See https://v8.dev/blog/math-random for details.

@micahstubbs
Copy link
Contributor Author

micahstubbs commented May 10, 2022

Second, we see some peer dep warnings for these packages:

npm WARN @swimlane/ngx-datatable@13.1.0 requires @angular/common@<7.0.0
npm WARN @swimlane/ngx-datatable@13.1.0 requires @angular/compiler@<7.0.0
npm WARN @swimlane/ngx-datatable@13.1.0 requires @angular/core@<7.0.0
npm WARN @swimlane/ngx-datatable@13.1.0 requires @angular/platform-browser@<7.0.0
npm WARN @swimlane/ngx-datatable@13.1.0 requires @angular/platform-browser-dynamic@<7.0.0
#
npm WARN @swimlane/ngx-graph@6.0.0 requires @angular/animations@6.x | 7.x | 8.x
npm WARN @swimlane/ngx-graph@6.0.0 requires @angular/common@6.x | 7.x | 8.x
npm WARN @swimlane/ngx-graph@6.0.0 requires @angular/core@6.x | 7.x | 8.x
npm WARN @swimlane/ngx-graph@6.0.0 requires @angular/cdk@6.x | 7.x | 8.x
#
npm WARN @swimlane/ngx-charts@8.1.0 requires @angular/cdk@6.x
npm WARN @swimlane/ngx-charts@8.1.0 requires @angular/common@6.x
npm WARN @swimlane/ngx-charts@8.1.0 requires @angular/core@6.x
npm WARN @swimlane/ngx-charts@8.1.0 requires @angular/animations@6.x
npm WARN @swimlane/ngx-charts@8.1.0 requires @angular/platform-browser@6.x

@micahstubbs
Copy link
Contributor Author

@micahstubbs
Copy link
Contributor Author

@micahstubbs
Copy link
Contributor Author

@micahstubbs
Copy link
Contributor Author

micahstubbs commented May 10, 2022

a few new peer dep warnings:

@swimlane/ngx-charts@11.0.1 requires @angular/animations@7.x | 8.x
@swimlane/ngx-charts@11.0.1 requires @angular/cdk@7.x | 8.x
@swimlane/ngx-charts@11.0.1 requires @angular/core@7.x | 8.x
@swimlane/ngx-charts@11.0.1 requires @angular/common@7.x | 8.x
@swimlane/ngx-charts@11.0.1 requires @angular/forms@7.x | 8.x
@swimlane/ngx-charts@11.0.1 requires @angular/platform-browser@7.x | 8.x
@swimlane/ngx-charts@11.0.1 requires @angular/platform-browser-dynamic@7.x | 8.x

let's try:

npm i @swimlane/ngx-charts@10.1.0

https://github.com/swimlane/ngx-charts/blame/edbdcc78dec8f59f661065948dbd6c65ffb1f3d2/package.json

That did the trick. It seems that @7.x | 8.x does not mean what it seems.

@micahstubbs
Copy link
Contributor Author

micahstubbs commented May 10, 2022

Update Angular Material to v7 by running:

NG_DISABLE_VERSION_CHECK=1 npx @angular/cli@7 update @angular/material@7

in your terminal. You should test your application for sizing and layout changes.

UPDATE package.json (3110 bytes)
up to date in 12.573s

It looks like it worked.

@micahstubbs
Copy link
Contributor Author

Here's what the checklist looks like right now:

https://update.angular.io/?l=3&v=6.1-7.2

Screen Shot 2022-05-10 at 4 03 53 PM

@micahstubbs
Copy link
Contributor Author

Remember this dialog? Let's test the UI manually and see if it is really broken like the update guide would imply (since ngModel should be removed in Angular 7).

Screen Shot 2022-05-10 at 2 42 06 PM

@micahstubbs
Copy link
Contributor Author

micahstubbs commented May 10, 2022

Ah hah! First we have to fix this TypeScript error. Let's do that.

client/app/cluster/cluster.component.ts:2:23 - error TS2305: Module 
'"/helix/helix-front/node_modules/@angular/flex-layout/flex-layout"' 
has no exported member 'ObservableMedia'.

2 import { MediaChange, ObservableMedia } from '@angular/flex-layout';
                        ~~~~~~~~~~~~~~~

[16:06:46] Found 1 error. Watching for file changes.

@micahstubbs
Copy link
Contributor Author

micahstubbs commented May 10, 2022

ObservableMedia was deprecated in 7.0.0-beta.19 and removed in 7.0.0-beta.23. Please use the nearly identical MediaObserver instead and consult the docs and CHANGELOG

angular/flex-layout#989

Hooray, that fixed it!

@micahstubbs
Copy link
Contributor Author

micahstubbs commented May 10, 2022

The login form still renders, so I guess that form change doesn't affect helix-front after all. Huh. Maybe it was just about existing Reactive Forms that used ngModel? 🤔 🤷

Screen Shot 2022-05-10 at 4 14 19 PM

@micahstubbs
Copy link
Contributor Author

Okay, let's test the upgrade from Angular 6.1 to Angular 7.2.

  • npm install works
  • npm rm type:check:watch shows 0 TypeScript errors
  • npm run lint runs and passes All files pass linting.
  • npm run build works
  • npm start works, with 0 errors in the browser console.
  • npm run test appears to work.

npm start works as expected, and recognizes our proxy config in proxy.conf.json.

Screen Shot 2022-05-10 at 4 16 46 PM

Helix UI renders with XX errors and YY warning in the browser console. The warning is understandable since the piwik telemetry library is not configured.

npm run test results

Screen Shot 2022-05-10 at 4 17 10 PM

@micahstubbs
Copy link
Contributor Author

micahstubbs commented May 10, 2022

Ahh, spoke too soon. We have a build error.

 npm run build

> helix-front@1.2.1 build /helix/helix-front
> rm -rf dist && mkdir dist && ng build --aot --prod && tsc -p server


Date: 2022-05-10T23:18:51.463Z
Hash: 71d2d6da0ff17adc81bd
Time: 13362ms
chunk {0} runtime.80ab492fe3d778817936.js (runtime) 1.41 kB [entry] [rendered]
chunk {1} main.08a4dae5096073b1e3b1.js (main) 128 bytes [initial] [rendered]
chunk {2} polyfills.f2ec7f3c988fb0dd4662.js (polyfills) 130 bytes [initial] [rendered]
chunk {3} styles.d0fa613a0ca6430dc8c0.css (styles) 88.3 kB [initial] [rendered]

ERROR in : 
Cannot determine the module for class AxisLabelComponent in 
/helix/helix-front/node_modules/
@swimlane/ngx-charts/release/common/axes/axis-label.component.d.ts! 
Add AxisLabelComponent to the NgModule to fix it.

Cannot determine the module for class XAxisTicksComponent in 
/helix/helix-front/node_modules/
@swimlane/ngx-charts/release/common/axes/x-axis-ticks.component.d.ts!
Add XAxisTicksComponent to the NgModule to fix it.

Cannot determine the module for class XAxisComponent in
/helix/helix-front/node_modules/
@swimlane/ngx-charts/release/common/axes/x-axis.component.d.ts!
Add XAxisComponent to the NgModule to fix it.

Cannot determine the module for class YAxisTicksComponent in
/helix/helix-front/node_modules/
@swimlane/ngx-charts/release/common/axes/y-axis-ticks.component.d.ts!
Add YAxisTicksComponent to the NgModule to fix it.

Cannot determine the module for class YAxisComponent in
/helix/helix-front/node_modules/
@swimlane/ngx-charts/release/common/axes/y-axis.component.d.ts! 
Add YAxisComponent to the NgModule to fix it.

github issue: swimlane/ngx-charts#829
fix: swimlane/ngx-charts#872

@micahstubbs
Copy link
Contributor Author

Here's the solution:

npm i @swimlane/ngx-charts@11.2.0

https://github.com/swimlane/ngx-charts/releases/tag/11.2.0

@micahstubbs
Copy link
Contributor Author

🎉 build successful

npm run build

> helix-front@1.2.1 build /helix/helix-front
> rm -rf dist && mkdir dist && ng build --aot --prod && tsc -p server


Date: 2022-05-10T23:38:02.749Z
Hash: 4aa0ec6cfe2eedfdc6ba
Time: 126980ms
chunk {0} runtime.80ab492fe3d778817936.js (runtime) 1.41 kB [entry] [rendered]
chunk {1} main.784c697c81a3874764be.js (main) 3.34 MB [initial] [rendered]
chunk {2} polyfills.9604bffe3fc17834fbe3.js (polyfills) 77.7 kB [initial] [rendered]
chunk {3} styles.d0fa613a0ca6430dc8c0.css (styles) 88.3 kB [initial] [rendered]

@micahstubbs
Copy link
Contributor Author

Okay, let's test the upgrade from Angular 6.1 to Angular 7.2. once more.

  • npm install works
  • npm rm type:check:watch shows 0 TypeScript errors
  • npm run lint runs and passes All files pass linting.
  • npm run build works
  • npm start works, with 0 errors in the browser console.
  • npm run test appears to work.

npm start works as expected, and recognizes our proxy config in proxy.conf.json.

Screen Shot 2022-05-10 at 4 16 46 PM

Helix UI renders with XX errors and YY warning in the browser console. The warning is understandable since the piwik telemetry library is not configured.

npm run test results

Screen Shot 2022-05-10 at 4 17 10 PM

micahstubbs added a commit to micahstubbs/helix that referenced this issue Jun 1, 2022
Fix security vulnerabilities in helix-front dependencies.
Upgrade helix-front dependencies to improve contributor productivity.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant