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: Angular 17 #116

Merged
merged 10 commits into from
Jul 10, 2024
Merged

Upgrade: Angular 17 #116

merged 10 commits into from
Jul 10, 2024

Conversation

yongkanm
Copy link
Contributor

@yongkanm yongkanm commented Jun 20, 2024

Description

This pull request upgrades the Angular version used by token-atm-spa from 15 to 17.

Breaking Changes and Deprecations Information for Project Dependencies

The following list includes links to information about breaking changes and deprecations of project dependencies that gets upgraded in this pull request:

Applied Migrations

  • (Ref) For TypeScript config, importsNotUsedAsValues is deprecated in favor of verbatimModuleSyntax since TypeScript 5.0. Setting verbatimModuleSyntax to true enforces the use of import type when only the declaration is used.

    • In tsconfig.json, importsNotUsedAsValues: "error" is replaced with verbatimModuleSyntax: true.
    • ESLint rule @typescript-eslint/consistent-type-imports is added to enforce and provide auto-fix for this constraint.
    • The use of @Inject() is required when injecting services from Angular DI in constructor (otherwise only type will be imported, which causes the failure of Angular DI at runtime).
  • (Ref) In angular.json, browserTarget is deprecated in favor of buildTarget (automatically changed by ng update migrations).

  • (Ref) TypeScript version upgrade results in a type check failure for type-fest@3.1.0, which is an dependency of decamelize-keys. This issue is resolved in newer versions of type-fest. Therefore, the type-fest version used by decamelize-keys is overridden in package.json.

  • Dependencies of custom-schematics have their versions bumped accordingly.

  • Angular Material version upgrade causes the following CSS rule being emitted, which results in the changing of warning style for single-selection fields and multi-selection fields:

    .mat-mdc-option.mdc-list-item--disabled .mat-mdc-option-pseudo-checkbox, .mat-mdc-option.mdc-list-item--disabled .mdc-list-item__primary-text, .mat-mdc-option.mdc-list-item--disabled>mat-icon {
        opacity: .38;
    }

    styles.sass is modified to unset this emitted CSS rule. This modification is necessary as the warning style for single-selection fields and multi-selection fields was implemented by manually overriding emitted CSS styles (it fails to be implemented by configuring themes for Angular Material). Achieving the same style by configuring Angular Material theme should be re-attempted later on for the upgraded Angular Material.

Testing Note

Please test if Token ATM still functions as expected after this Angular major version upgrade. Here are some recommendations for testing:

  • Go through links included in the Breaking Changes and Deprecations Information for Project Dependencies section above and verify if any migrations other than those mentioned in the Applied Migrations section is necessary.

  • Check Angular Update Guide, which provides an interactive checklist for migrations of Angular and Angular Material between major versions.

  • Perform brief functionality testing for Token ATM.

    • Note: As mentioned by Display Grid View in New Window Stops Working #115, the feature of displaying grid view in new window stops working. This is not caused by the Angular major version upgrade and could be reproduced with the main branch. It will be fixed in another pull request. Please ignore this feature when performing testing.

@yongkanm yongkanm self-assigned this Jun 20, 2024
@yongkanm yongkanm requested a review from epsilonError June 20, 2024 19:34
@yongkanm yongkanm changed the base branch from refactor-token-option-creation to main June 20, 2024 19:36
@yongkanm yongkanm force-pushed the upgrade-angular-17 branch from 5c087ad to b96c698 Compare June 27, 2024 01:17
@epsilonError
Copy link
Contributor

epsilonError commented Jul 1, 2024

Angular Update Guide Checklist

Checklist from Angular to update from v15 to v17.

Angular v16 Upgrade

  • Angular v16 supports node.js versions: v16 and v18. [Using node 18 in devcontainer]
  • Angular v16 supports TypeScript version 4.9.3 or later. [Using ~5.4.5 in token-atm-spa & custom-schematics]
  • Use @angular/core@16 & @angular/cli@16 for Angular v16. [Using core@17 and cli@17]
  • Run ng update @angular/material@16. [Using material@17]
  • Angular v16 supports Zone.js version 0.13.x or later. [Using zone.js 0.14.x]
  • The Event union no longer contains RouterEvent, which means that if you're using the Event type you may have to change the type definition from (e: Event) to (e: Event|RouterEvent) [Only found single use of Event type (onSelectFile()), which isn't expected to handle RouterEvents]
  • In addition to NavigationEnd the routerEvent property now also accepts type NavigationSkipped [No routerEvent properties found.]
  • Pass only flat arrays to RendererType2.styles because it no longer accepts nested arrays [No RendererType2.styles uses found.]
  • You may have to update tests that use BrowserPlatformLocation because MockPlatformLocation is now provided by default in tests. [No BrowserPlatformLocation uses found.]
  • Due to the removal of the Angular Compatibility Compiler (ngcc) in v16, projects on v16 and later no longer support View Engine libraries. [Doesn't seem to be used by us... But @angular/compiler-cli does include a bundle of ngcc...]
  • After bug fixes in Router.createUrlTree you may have to readjust tests which mock ActiveRoute. [No uses or mocks of ActiveRoute found.]
  • Change imports of ApplicationConfig to be from @angular/core. [No imports from ApplicationConfig found]
  • Revise your code to use renderModule instead of renderModuleFactory because it has been deleted. [No uses of renderModuleFactory found.]
  • Revise your code to use XhrFactory from @angular/common instead of XhrFactory export from @angular/common/http. [No uses of XhrFactory found.]
  • If you're running multiple Angular apps on the same page and you're using BrowserModule.withServerTransition({ appId: 'serverApp' }) make sure you set the APP_ID instead since withServerTransition is now deprecated. [No uses of withServerTransition found.]
  • Change EnvironmentInjector.runInContext to runInInjectionContext and pass the environment injector as the first parameter. [No uses of runInContext found.]
  • Update your code to use ViewContainerRef.createComponent without the factory resolver. ComponentFactoryResolver has been removed from Router APIs. [No uses of ComponentFactoryResolver found. But createComponent is used in concatenate, list, and switch field components. The functionality of these components will be specifically tested later.]
  • If you bootstrap multiple apps on the same page, make sure you set unique APP_IDs. [No APP_IDS found. But token-atm-spa is run as a single app.]
  • Update your code to revise renderApplication method as it no longer accepts a root component as first argument, but instead a callback that should bootstrap your app. [No uses of renderApplication found.]
  • Update your code to remove any reference to PlatformConfig.baseUrl and PlatformConfig.useAbsoluteUrl platform-server config options as it has been deprecated. [No uses of either Config Url were found.]
  • Update your code to remove any reference to @Directive/@Component moduleId property as it does not have any effect and will be removed in v17. [moduleId seems to be only used in the context of a Canvas moduleID. More research required.]
  • Update imports from import {makeStateKey, StateKey, TransferState} from '@angular/platform-browser' to import {makeStateKey, StateKey, TransferState} from '@angular/core' [No imports of makeStateKey, StateKey, TransferState from @angular/platform-browser found.]
  • If you rely on ComponentRef.setInput to set the component input even if it's the same based on Object.is equality check, make sure you copy its value. [No uses of setInput found.]
  • Update your code to remove any reference to ANALYZE_FOR_ENTRY_COMPONENTS injection token as it has been deleted. [No references to ANALYZE_FOR_ENTRY_COMPONENTS found.]
  • entryComponents is no longer available and any reference to it can be removed from the @NgModule and @Component public APIs. [No references to entryComponents found.]
  • ngTemplateOutletContext has stricter type checking which requires you to declare all the properties in the corresponding object. [No uses of ngTemplateOutletContext found.]
  • Angular packages no longer include FESM2015 and the distributed ECMScript has been updated from 2020 to 2022. [No reference to FESM2015 and ECMScript found.]
  • The deprecated EventManager method addGlobalEventListener has been removed as it is not used by Ivy. [No uses of addGlobalEventListener found.]
  • BrowserTransferStateModule is no longer available and any reference to it can be removed from your applications. [No references to BrowserTransferStateModule found.]
  • Update your code to use Injector.create rather than ReflectiveInjector since ReflectiveInjector is removed. [No uses of ReflectiveInjector found.]
  • QueryList.filter now supports type guard functions. Since the type will be narrowed, you may have to update your application code that relies on the old behavior. [No uses of QueryList found.]

Angular v17 Upgrade

  • Angular v17 supports node.js versions: v18.13.0 and newer [All node versions in both package-lock.json seem to allow >= version 18.13.0 of node. Devcontainer installs version 18.]
  • Make sure that you are using a supported version of TypeScript before you upgrade your application. Angular v17 supports TypeScript version 5.2 or later. [Packages use ~5.4.5, so 5.4.x will be used. (Some peer dependencies require <5.5 and the devDependency prettier-eslint uses it's own submodule with version ^4.5.4 of TypeScript)]
  • Angular v17 supports Zone.js version 0.14.x or later. [Using zone.js 0.14.x]
  • Use @angular/core@17 & @angular/cli@17 for Angular v17. [Using core@17 and cli@17]
  • Run ng update @angular/material@17. [Using material@17]
  • Angular now automatically removes styles of destroyed components, which may impact your existing apps in cases you rely on leaked styles. To change this update the value of the REMOVE_STYLES_ON_COMPONENT_DESTROY provider to false. [Requires more testing...]
  • Make sure you configure setupTestingRouter, canceledNavigationResolution, paramsInheritanceStrategy, titleStrategy, urlUpdateStrategy, urlHandlingStrategy, and malformedUriErrorHandler in provideRouter or RouterModule.forRoot since these properties are now not part of the Router's public API [Does not seem to be configured. Requires more testing...]
  • For dynamically instantiated components we now execute ngDoCheck during change detection if the component is marked as dirty. You may need to update your tests or logic within ngDoCheck for dynamically instantiated components. [No uses of ngDoCheck found. Functionality requires more testing...]
  • Handle URL parsing errors in the UrlSerializer.parse instead of malformedUriErrorHandler because it's now part of the public API surface. [No uses of malformedUriErrorHandler found.]
  • Change Zone.js deep imports like zone.js/bundles/zone-testing.js and zone.js/dist/zone to zone.js and zone.js/testing. [No Zone.js deep imports found.]
  • You may need to adjust your router configuration to prevent infinite redirects after absolute redirects. In v17 we no longer prevent additional redirects after absolute redirects. [Requires more testing...]
  • Change references to AnimationDriver.NOOP to use NoopAnimationDriver because AnimationDriver.NOOP is now deprecated. [No references to AnimationDriver.NOOP found.]
  • You may need to adjust the equality check for NgSwitch because now it defaults to stricter check with === instead of ==. Angular will log a warning message for the usages where you'd need to provide an adjustment. [No uses of NgSwitch found. But requires more testing...]
  • Use update instead of mutate in Angular Signals. For example items.mutate(itemsArray => itemsArray.push(newItem)); will now be items.update(itemsArray => [itemsArray, …newItem]); [No uses of .mutate( found.]
  • To disable hydration use ngSkipHydration or remove the provideClientHydration call from the provider list since withNoDomReuse is no longer part of the public API. [No uses of withNoDomReuse found.]
  • If you want the child routes of loadComponent routes to inherit data from their parent specify the paramsInheritanceStrategy to always, which in v17 is now set to emptyOnly. [paramsInheritanceStrategy is not specified. Requires more testing...]

@epsilonError
Copy link
Contributor

epsilonError commented Jul 2, 2024

Breaking Changes and Deprecation Checks

Typescript API Breaking Changes (ts 5.0 to 5.3)

  • Requires Node 14.7 or newer [Using Node 18]
  • TypeScript targets ES2020 [token-atm-spa: ES2022, custom-schematics: es6 and lib: es2018]
  • No other references to items from API Breaking Change found.

TypeScript 5.4 Announcement

  • No uses of these deprecated options and behaviors found.
  • Typed interfaces and properties of DOM objects have changed [Requires UI testing to check...]

Angular (upgrade from 15 to 17)

See previous comment following Angular Update Guide Checklist.

Angular Components

  • Requires more testing...

Angular ESLint (upgrade from 15 to 17)

  • Node must be version 18.13.0 or higher. [Using node 18, my devcontainer install uses v18.20.3.]
  • No references to items with breaking changes found.
  • Now uses typescript-eslint, which will be checked below

TypeScript ESLint (upgrade from 5 to 7)

Need more research to check:

  • What "eslint-plugin: Adds an additional class of checks to the rule" have been added to v6?
  • What are the new "ESLint, NodeJS, and TS minimum version requirements" for v7?

Angular CLI (upgrade from 15 to 17)

  • @angular-devkit/build-angular "Deprecated outputPath and outputPaths from the server and browser builder have been removed from the builder output. Use outputs instead. Note: this change does not effect application developers." [Requires testing to confirm token-atm-spa still builds to "dist/token-atm-spa" Correct Build Directory CONFIRMED]
  • References to breaking changes not found in token-atm-spa. Not all deprecations were checked, will test later while working through functionality testing.

Angular Bootstrap by Valor Software (upgrade from 10.2.0 to 12.0.0)

  • TBD (Needs more research, requires code comparison...)

ESLint (upgrade from 8.33.0 to 8.57.0)

  • Nothing to check. Needs more research...

@epsilonError
Copy link
Contributor

epsilonError commented Jul 2, 2024

npm outdated Checks

Known correct @angular/typescript versions have been removed.

The packages below may or may not need to be updated as part of this Angular update pull request.

I expect jasmine, node, karma, eslints, and prettier may be part of a fresh install of Angular v17 and may be worth checking the most up-to-date version compatible with Angular v17.

Other packages are included as a shortlist for checking if any functionality errors occur.

For custom-schematics

Package Current Wanted Latest Location Depended by
@types/jasmine 4.3.6 4.3.6 5.1.4 node_modules/@types/jasmine custom-schematics
@types/node 14.18.63 14.18.63 20.14.9 node_modules/@types/node custom-schematics
jasmine 4.5.0 4.6.0 5.1.0 node_modules/jasmine custom-schematics

For token-atm-spa

Package Current Wanted Latest Location Depended by
@types/jasmine 4.3.6 4.3.6 5.1.4 node_modules/@types/jasmine dev-con-test
@types/lodash 4.17.5 4.17.6 4.17.6 node_modules/@types/lodash dev-con-test
@types/uuid 9.0.8 9.0.8 10.0.0 node_modules/@types/uuid dev-con-test
@typescript-eslint/eslint-plugin 7.13.1 7.15.0 7.15.0 node_modules/@typescript-eslint/eslint-plugin dev-con-test
@typescript-eslint/parser 7.13.1 7.15.0 7.15.0 node_modules/@typescript-eslint/parser dev-con-test
ag-grid-angular 30.2.1 30.2.1 32.0.0 node_modules/ag-grid-angular dev-con-test
ag-grid-community 30.2.1 30.2.1 32.0.0 node_modules/ag-grid-community dev-con-test
camelcase-keys 8.0.2 8.0.2 9.1.3 node_modules/camelcase-keys dev-con-test
compress-json 2.1.3 2.1.3 3.1.0 node_modules/compress-json dev-con-test
date-fns 2.30.0 2.30.0 3.6.0 node_modules/date-fns dev-con-test
eslint 8.57.0 8.57.0 9.6.0 node_modules/eslint dev-con-test
eslint-config-prettier 8.10.0 8.10.0 9.1.0 node_modules/eslint-config-prettier dev-con-test
eslint-plugin-json 3.1.0 3.1.0 4.0.0 node_modules/eslint-plugin-json dev-con-test
eslint-plugin-markdown 3.0.1 3.0.1 5.0.0 node_modules/eslint-plugin-markdown dev-con-test
eslint-plugin-prettier 4.2.1 4.2.1 5.1.3 node_modules/eslint-plugin-prettier dev-con-test
fp-ts 2.16.6 2.16.7 2.16.7 node_modules/fp-ts dev-con-test
html-dom-parser 3.1.7 3.1.7 5.0.8 node_modules/html-dom-parser dev-con-test
husky 8.0.3 8.0.3 9.0.11 node_modules/husky dev-con-test
jasmine-core 4.5.0 4.5.0 5.1.2 node_modules/jasmine-core dev-con-test
karma-chrome-launcher 3.1.1 3.1.1 3.2.0 node_modules/karma-chrome-launcher dev-con-test
karma-jasmine-html-reporter 2.0.0 2.0.0 2.1.0 node_modules/karma-jasmine-html-reporter dev-con-test
prettier 2.8.8 2.8.8 3.3.2 node_modules/prettier dev-con-test
prettier-eslint 15.0.1 15.0.1 16.3.0 node_modules/prettier-eslint dev-con-test
uuid 9.0.1 9.0.1 10.0.0 node_modules/uuid dev-con-test

Notes

  • Jasmine Upgrade to 5.0 guide
  • I'm having a hard time teasing out all the ESLint and prettier versions, setup, and use information. It's not a blocker for this pull request but it may be a good idea to look into what is actually needed in future.

@yongkanm
Copy link
Contributor Author

yongkanm commented Jul 5, 2024

Thank you for checking all the packages! For this pull request my preference is to minimize changes, so I did not update any package that isn't required to be updated. Here are some thoughts about Jasmine, TypeScript, and Prettier:

Angular 17 itself is using Jasmine 5 (according to the package.json at the root level of Angular monorepo), but Angular doesn't seem to have a requirement for the Jasmine version used by projects that use Angular. Since Angular uses peerDependency to control dependency compatibility, I didn't update Jasmine given that peerDependency is not broken for Token ATM.

TypeScript is a bit tricker. Since it is a compile-time dependency most of the time (linter is an exception), version mismatch is not necessarily a problem. The only thing to worry about is the compatibility of exported declaration files, and that's why I override the type-fest version for decamelize-keys. It would be much more complicated to check TypeScript compatibility for each package, so I decided to just aim for passing the compilation. It might indeed cause some issues in the future since TypeScript only performs lib checks on imported modules. If we import new modules whose declarations are not compatible with TypeScript 5 from existing dependencies, we might need to update those dependencies to solve this issue.

Also, thank you for catching the Prettier issue! I misremembered it as added by Angular, but now I realize that it was manually added by us at the very beginning. Also, prettier and prettier-eslint does not list typescript as their peerDependency, so the compatibility needs to be manually managed. Prettier has backported TypeScript 5 support in v2.8.5, so the currently installed version (v2.8.8 from package-lock.json) does work with TypeScript 5. However, package.json specifies prettier version as ^2.8.4, so I want to update it to ^2.8.8. As you mentioned that it is hard to figure out the compatibility between eslint, prettier, and prettier-eslint, I think it would be better to not concern about upgrading Prettier to v3 in this pull request.

I will push a commit soon to update prettier version in package.json.

@epsilonError
Copy link
Contributor

epsilonError commented Jul 7, 2024

1. Token ATM Pages

  • 1. Login Page
  • 1.1. Hover Tooltip
  • 1.2. Password Input
  • 1.3. Configure Optional Credential Modal
  • 1.3.1. Qualtrics
  • 1.3.2. Question Pro
  • 1.4. Save/Overwrite Saved Credentials
  • 1.5. Credential Help Pop-up
  • 1.6. Login Button
  • 1.7. Unlock Saved Credentials
  • 2. Course Selection Page
  • 2.1. Search Input Fields
  • 2.2. Course Selection Choices
  • 3. Dashboard Sidebar
  • 3.1. Logo
  • 3.2. Profile Info
  • 3.3. Course Info
  • 3.4. Switch Course
  • 3.5. Token ATM Pages
  • 4. Process Request Page
  • 4.1. Start Processing button
  • 4.1.1. Gather Submissions
  • 4.1.2. Resolve Requests
  • 4.1.3. Handle Requests
  • 4.1.4. Token ATM Log Not Published Warning
  • 4.1.5. Missing Optional Credential Redirect/Warning
  • 5. Token Options Page
  • 5.1. Switch to Grid View
  • 5.1.1. No Configured Columns Notification
  • 5.2. Open Grid View in new Window (Doesn't Display, but window opens as expected)
  • 5.2.1. No Configured Columns Notification
  • 5.3. Configure Grid View
  • 5.4. Export Processed Requests (All, Token Option Group, or Individual Token Option)
  • 5.4.1. Scope
  • 5.4.2. Time Range
  • 5.4.3. Included Reject Requests
  • 5.5. Make New Token Option Group
  • 5.6. Token Option Group Display
  • 5.6.1. Publish Button
  • 5.6.2. Add New Token Option
  • 5.6.2.1. Selection Modal
  • 5.6.2.2. Missing Optional Credential Redirect/Warning
  • 5.6.3. Group Dropdown
  • 5.6.3.1. Edit
  • 5.6.3.2. Export
  • 5.6.3.3. Delete
  • 5.6.4. Individual Token Option Display
  • 5.6.4.1. Open Individual Token Option Management
  • 5.6.4.1.1. On Edit, Missing Optional Credential Redirect/Warning
  • 5.6.4.2. Individual Token Option Dropdown
  • 5.6.4.2.1. Move
  • 5.6.4.2.2. Export
  • 5.6.4.2.3. Delete
  • 6. Students Page
  • 6.1. Import CSV for Batch Processing
  • 6.1.1. File Upload Field
  • 6.1.2. Cancel / Close
  • 6.1.3. Import
  • 6.1.4. Download Error CSV
  • 6.2. Student Search
  • 6.3. Student Pagination
  • 6.4. Individual Student Info
  • 6.4.1. Student Info
  • 6.4.2. Manual Token Balance Adjustment
  • 6.4.3. Make Request on Student's Behalf
  • 6.4.4. Student's Processed Requests Displayed
  • 7. Configuration Page
  • 8. Dev-Test Page

2. Miscellaneous Components

  • 1. Retrieving Info from Canvas Toast
  • 2. Caught Error Modal
  • 3. Uncaught Global Error Pop-up
  • 4. HTTP Retry Toast

3. Form field Components

  • Labelled Field
  • Token Option Id Field
  • Exclude Token Options Field
  • Withdraw Token Option Field
  • Single Selection Quiz Field
  • Start Time Field
  • End Time Field
  • New (Due) Time Field
  • Multiple Section Date Field
  • - List of optional Multi-Selection Sections Field
  • Grade Threshold Field
  • Single Selection Assignment Field
  • Single Selection Assignment Group Field
  • Default Token Option Fields (Name, Description, Token Balance Change)
  • Overall/Catch-all Validation Errors Wrapper
  • Optional Checkbox Field
  • Switch Choice Field
  • Token Option Group Management
  • Token Option Management
  • Basic Token Option
  • Earn By Module Token Option
  • Earn by Question Pro Quiz Token Option
  • - Lazy Single Selection Field
  • Earn By Canvas Quiz Token Option
  • Earn By Qualtrics Survey Token Option [Form Works, but can't test functionality]
  • Placeholder Token Option
  • Spend for Additional Points Token Option
  • - Add Canvas Score Field
  • Spend for No Longer Marked Late Canvas Assignment Extension Token Option
  • Spend for Assignment Resubmission (Change Available Until) Token Option
  • Spend for Lab Data (Change Quizzes Available Until) Token Option
  • Spend for Lab Switch Token Option
  • Spend for Passing Assignment (Make Grade match Threshold) Token Option
  • Spend for Quiz Revision after Missing Threshold Token Option
  • Withdraw Assignment Resubmission Token Option
  • Withdraw Lab Data Token Option
  • Withdraw Lab Switch Token Option

@epsilonError
Copy link
Contributor

epsilonError commented Jul 8, 2024

Note: I had a Production build problem since I'm running a local server for Token ATM.

To make it work I built Token ATM in production mode, served the website from the /dist folder, renamed /dist/token-atm-spa to /dist/token-atm, and loaded token-atm-app's window.location.href as "http://localhost:4200/token-atm".

I believe this is the same/similar changes github actions makes when bundling and publishing the token-atm website. But I thought I should leave a comment just to make sure.

Copy link
Contributor

@epsilonError epsilonError left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brief Functionality Testing resulted in no errors. Angular 17 upgrade looks good to me.

I didn't do a thorough testing of all the guards, but I don't expect their execution changed.

@yongkanm yongkanm merged commit cb1c1d7 into main Jul 10, 2024
1 check passed
@epsilonError epsilonError deleted the upgrade-angular-17 branch November 7, 2024 18:45
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 this pull request may close these issues.

2 participants