Wave 1: Upgrade core framework + build config from Angular 9 to Angular 21#421
Wave 1: Upgrade core framework + build config from Angular 9 to Angular 21#421tobydrinkall wants to merge 2 commits into
Conversation
Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| "module": "preserve", | ||
| "moduleResolution": "bundler", |
There was a problem hiding this comment.
📝 Info: Removal of emitDecoratorMetadata is safe for Ivy-based Angular
The PR removes emitDecoratorMetadata from tsconfig.json. This is correct because Angular's Ivy compiler (default since Angular 9) does not rely on TypeScript's emitDecoratorMetadata for dependency injection - it uses its own compile-time analysis instead. This removal is safe and aligns with modern Angular project configurations.
Was this helpful? React with 👍 or 👎 to provide feedback.
Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>
There was a problem hiding this comment.
🚩 Deprecated tooling configs retained (tslint, protractor)
The angular.json still references @angular-devkit/build-angular:tslint (angular.json:106) and @angular-devkit/build-angular:protractor (angular.json:119). Both tslint and protractor have been deprecated for years — tslint was superseded by eslint, and protractor was removed from Angular CLI in v15. The package.json still lists tslint (package.json:47) and protractor (package.json:45) as devDependencies. While these won't break the build, ng lint and ng e2e will likely fail at runtime since the builders may not exist in @angular-devkit/build-angular v21. Consider migrating to @angular-eslint and a modern e2e framework like Cypress or Playwright.
(Refers to lines 105-129)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Intentional: the TSLint → ESLint migration and removal of the protractor e2e setup are Wave 4 of this migration and are being handled in a follow-up PR based on this branch. This foundational PR only targets a green npm run build.
| } | ||
| } | ||
| }, | ||
| "defaultConfiguration": "production" |
There was a problem hiding this comment.
📝 Info: Build default changed from development to production
The build target now has "defaultConfiguration": "production" (angular.json:68), meaning ng build (and therefore npm run build) will produce a production build by default. Previously with the browser builder and no defaultConfiguration, ng build ran without any configuration applied. This is the standard convention for modern Angular projects but is a behavioral change worth noting — all ng build invocations will now apply production optimizations, file replacements, and service worker registration unless explicitly overridden.
Was this helpful? React with 👍 or 👎 to provide feedback.
| }, | ||
| "hosting": { | ||
| "public": "dist", | ||
| "public": "dist/angular-hnpwa/browser", |
There was a problem hiding this comment.
📝 Info: Application builder output path structure change affects deployment
The application builder outputs client files to dist/angular-hnpwa/browser/ rather than the old browser builder's dist/angular-hnpwa/. The firebase.json was correctly updated to dist/angular-hnpwa/browser (firebase.json:6). However, any other deployment scripts, CI pipelines, or documentation referencing the old dist/ or dist/angular-hnpwa/ path would need updating. The CONTRIBUTING.md at step 10 references updating the dist/ directory, which is now stale but is guidance text rather than executable code.
Was this helpful? React with 👍 or 👎 to provide feedback.
| "rxjs-compat": "^6.5.2", | ||
| "tslib": "^1.10.0", | ||
| "tslib": "^2.6.0", | ||
| "unfetch": "^4.1.0", |
There was a problem hiding this comment.
📝 Info: unfetch dependency may conflict with esbuild-based application builder
The hackernews-api.service.ts uses import fetch from 'unfetch' (src/app/shared/services/hackernews-api.service.ts:3) as a fetch polyfill. The application builder uses esbuild internally, which handles module resolution differently than webpack. The unfetch package uses a default export pattern that should work with esModuleInterop: true (added in this PR's tsconfig changes), but unfetch is also unnecessary in modern browsers — the native fetch API is available in all evergreen browsers. This dependency could be removed entirely.
Was this helpful? React with 👍 or 👎 to provide feedback.
E2E Test Results — Angular 21 upgrade (waves 1–4 integrated)Tested by merging all four wave branches (#421 + #422 + #423 + #424) into a local integration branch, running build/tests/lint, then clicking through the app served via All tests passed:
More evidence
Shell evidence (integration branch):
|
1 participant
Summary
Foundational PR of a 4-wave Angular 9 → 21 migration (Angular 21 is the latest version supported by the repo's Node 22.12 toolchain; Angular 22 requires Node ≥22.22). This wave gets
npm run buildgreen on Angular 21; follow-up waves handle RxJS/TS API changes, component/template fixes, and test+lint modernization on top of this branch.Key changes:
package.json: all@angular/*~9.0.x→^21.0.0,typescript ~3.7.5→~5.9.2,zone.js ~0.10→~0.16,tslib1 → 2; karma toolchain bumped to versions required by build-angular 21 peer deps (karma 6.4, jasmine-core 5, karma-coverage replaces karma-coverage-istanbul-reporter);codelyzerremoved (incompatible, ESLint migration lands in wave 4)angular.json: webpackbrowserbuilder → esbuildapplicationbuilder (main→browser, dropped removed optionsextractCss/aot/buildOptimizer/vendorChunk,serviceWorkernow takes the ngsw config path); dev-serverbrowserTarget→buildTarget; addeddevelopmentconfiguration +defaultConfigurationstandalone: falseto all 11 components and theCommentPipe(standalone became the default in v19; app stays NgModule-based)src/test.tsdeleted; test target now usespolyfills: ["zone.js", "zone.js/testing"](esbuild karma builder auto-discovers specs);zone.js/dist/zonedeep import →zone.jstarget es2022,moduleResolution bundler,skipLibCheck(needed while rxjs 6 typings remain)Intentionally deferred: rxjs stays at 6.5 + rxjs-compat (deep
rxjs/Observableimports still compile) — wave 2; Sass@import/division deprecation warnings — wave 3;ng lint(tslint builder no longer exists) and spec/karma updates — wave 4.Verified locally:
npm installandnpm run buildsucceed.Link to Devin session: https://app.devin.ai/sessions/11374427e0fc4cf6acdf4838f7a9651c
Requested by: @tobydrinkall