-
Notifications
You must be signed in to change notification settings - Fork 5
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 to Angular 14 #738
Conversation
76a7c63
to
0a72446
Compare
Finding a significant issue with running the tests, almost half of them fail now because of importing
The
|
Issues with running the tests don't occur in the |
I've been battling this all afternoon... I was hoping you might know the answer! |
This reverts commit 626aa2f.
Reverted too much... not quite there... |
@pjmonks I think this is now ready for review. |
Found an issue with the Bulk Editor which seems to be new as part of this upgrade, but can be fixed separately. See issue #739 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The branch now builds successfully. I've had a quick test clicking around the UI, only found one regression bug which I've created as a separate issue, but other than that cannot find any other issues.
I've requested one change to the config files to not break developer workflow, but once that's done I think I can approve it.
@@ -9,7 +9,7 @@ | |||
"dist-archive": "tar zcvf dist/mdm-ui-$npm_package_version$([ -z \"$MDM_UI_THEME_NAME\" ] && echo \"\" || echo \".$MDM_UI_THEME_NAME\").tgz -C dist mdm-ui-$npm_package_version$([ -z \"$MDM_UI_THEME_NAME\" ] && echo \"\" || echo \"-$MDM_UI_THEME_NAME\")", | |||
"test": "ng test", | |||
"test:watch": "ng test --watch", | |||
"test-clearCache": "ng test --clearCache", | |||
"test-clearCache": "ng test --clear-cache", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a small problem when running npm run license-check
command locally. Now that Angular 14 compiles intermediate files to a .angular
cache folder, the license-check library will check in this folder too for missing license headers. This wasn't noticed on the Jenkins build I think because of the license check happening before a compile/test.
Please update the license-check-and-add-config.json
file like below:
{
"ignore": [
".angular"
// ...
]
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic catch! I've pushed this change.
@@ -34,7 +34,7 @@ import { MatFormFieldAppearance } from '@angular/material/form-field'; | |||
export class CatalogueSearchFormComponent implements OnInit { | |||
@Input() appearance: MatFormFieldAppearance = 'outline'; | |||
@Input() routeSearchTerm?: string = ''; | |||
formGroup: FormGroup = new FormGroup({}); | |||
formGroup: UntypedFormGroup = new UntypedFormGroup({}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've heard about these new typed Angular Forms, would be interesting to refactor some of these at a later date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - the automatic migration didn't work for me so I had to figure this out myself, and decided Untyped versions would be best for now. But I've added a ticket #742 for sorting it out later
Thanks again for your really thorough reviewing on this, @pjmonks ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is in a great state to accept now, nice one! 🎉 I'll approve the PR but will give you the honour of merging 🏆
@pjmonks Oh you're too kind! Thanks! |
Hi, |
This is the final PR in the series - completing the upgrade to Angular 14. As part of this PR we also upgrade Chart JS for the summary metadata charts (I couldn't resist a minor style tweak at the same time), and upgrade a number of other packages for consistency.
The PR for upgrading Jodit should ideally be accepted before this one, so leaving it in draft mode for now.
Fixes #608
Fixes #553
Fixes #554