search

apache / helix

Mirror of Apache Helix
Apache License 2.0
466 stars 227 forks source link

upgrade helix-front to Angular 7.2 #2084

Closed micahstubbs closed 2 years ago

micahstubbs commented 2 years ago

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

see #2053 for motivation, details, and history.

micahstubbs commented 2 years ago

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 commented 2 years ago

https://stackoverflow.com/a/49918631/1732222

Screen Shot 2022-05-08 at 9 56 21 AM
micahstubbs commented 2 years ago

https://angular.io/api/forms/FormControlName#use-with-ngmodel-is-deprecated

micahstubbs commented 2 years ago

What are Reactive Forms? How do they work?

https://angular.io/guide/reactive-forms

https://www.digitalocean.com/community/tutorials/angular-reactive-forms-introduction

micahstubbs commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago 
npm run test
# TOTAL: 74 SUCCESS

passes, so that is a good sign.

micahstubbs commented 2 years ago

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 commented 2 years ago

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

Screen Shot 2022-05-10 at 2 42 06 PM
micahstubbs commented 2 years ago

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

micahstubbs commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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: https://github.com/apache/helix/issues/2053#issuecomment-1117996325

micahstubbs commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

https://github.com/angular/flex-layout/commit/b2e2064184b9eedb4c6d5f7e9ea6dbdbc4a667a3

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

micahstubbs commented 2 years ago

Next, let's update tsickle.

Here's the commit we want git blame

npm i tsickle@0.34.2
micahstubbs commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago 
npm i @swimlane/ngx-datatable@14.0.0

https://github.com/swimlane/ngx-datatable/blame/d7df15a070282a169524dba51d9572008b74804c/package.json

micahstubbs commented 2 years ago 
npm i @swimlane/ngx-charts@11.0.1

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

micahstubbs commented 2 years ago 
npm i @swimlane/ngx-graph@6.0.0-rc.2

https://github.com/swimlane/ngx-graph/blame/d5a78e45d320ffd976b78624e3d3d0027ca396ce/package.json

micahstubbs commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

https://github.com/angular/flex-layout/issues/989

Hooray, that fixed it!

micahstubbs commented 2 years ago

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 commented 2 years ago

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

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 commented 2 years ago

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: https://github.com/swimlane/ngx-charts/issues/829 fix: https://github.com/swimlane/ngx-charts/pull/872

micahstubbs commented 2 years ago

Here's the solution: 

npm i @swimlane/ngx-charts@11.2.0

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

micahstubbs commented 2 years ago

🎉 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 commented 2 years ago

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

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