Wave 4: Test suite fixes and TSLint -> ESLint migration#423
Wave 4: Test suite fixes and TSLint -> ESLint migration#423tobydrinkall wants to merge 3 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}
| @@ -1,11 +1,11 @@ | |||
| import { Injectable } from '@angular/core'; | |||
| import { Injectable, OnDestroy } from '@angular/core'; | |||
There was a problem hiding this comment.
📝 Info: OnDestroy on a root-provided service has limited practical effect
The SettingsService is providedIn: 'root' (line 6), meaning it lives for the entire application lifetime. Angular only calls ngOnDestroy on root services when the root injector is destroyed (i.e., when the app shuts down). Adding implements OnDestroy here is technically correct but provides no practical cleanup benefit during normal application use - the event listener will persist for the app's lifetime regardless. This is not a bug, but worth noting that the OnDestroy implementation gives an impression of lifecycle management that doesn't materially help.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Agreed this has limited practical effect for a root-provided service; the implements OnDestroy was added by ESLint's use-lifecycle-interface auto-fix since the pre-existing ngOnDestroy() method was already there. Keeping it for correctness.
| "test": "ng test", | ||
| "lint": "ng lint", | ||
| "e2e": "ng e2e" | ||
| "lint": "ng lint" |
There was a problem hiding this comment.
🚩 Protractor e2e tests removed without replacement
The PR removes all Protractor-based e2e tests (e2e/ directory) and the e2e npm script. While Protractor is deprecated and this removal is expected during modernization, no replacement e2e framework (Cypress, Playwright, etc.) is introduced. The new unit tests (comment.pipe.spec.ts, settings.service.spec.ts) partially compensate but don't cover integration/e2e scenarios. This may be intentional if e2e is planned for a follow-up, but worth confirming.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Intentional — Protractor is EOL and its removal (without a replacement in this PR) was explicitly in scope for Wave 4. A modern e2e framework can be added in a follow-up if desired.
Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>
|
|
||
| darkColorSchemeMedia = window.matchMedia('(prefers-color-scheme: dark)'); | ||
|
|
||
| private boundColorSchemeChangeHandler = this.handleSystemPreferredColorSchemeChange.bind(this); |
There was a problem hiding this comment.
📝 Info: Event listener fix correctly resolves a pre-existing cleanup bug
The old code called .bind(this) inline in both subscribeToSystemPreferredColorScheme and unSubscribeToSystemPrefferedColorScheme, which meant removeEventListener received a different function reference than addEventListener and thus never actually removed the listener. The new code at src/app/shared/services/settings.service.ts:19 stores the bound handler once, so removeEventListener at line 64 now correctly removes the same function that was added at line 43. This is a correct and important fix.
Was this helpful? React with 👍 or 👎 to provide feedback.
Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>
| @@ -0,0 +1,59 @@ | |||
| // @ts-check | |||
| const eslint = require("@eslint/js"); | |||
| const { defineConfig } = require("eslint/config"); | |||
There was a problem hiding this comment.
📝 Info: ESLint config uses defineConfig from eslint/config subpath export
The eslint.config.js imports defineConfig from eslint/config (eslint.config.js:3). This subpath export was introduced in ESLint v9.x and is available in the resolved v10.6.0 (per package-lock.json). However, the @eslint/js package is pinned to ^10.0.1 while eslint is ^10.3.0 — these are separate packages and don't strictly need version alignment, but keeping them in sync is a common recommendation to avoid subtle rule behavior differences.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Info noted — these versions were generated by ng add @angular-eslint/schematics, so they match what angular-eslint 21.4.0 expects; leaving as-is.
1 participant
Summary
Wave 4 of the Angular 9→21 migration: test + lint modernization on top of the Wave 1 core-upgrade branch.
ng testexited 1 with an empty suite. Added minimal specs (comment.pipe.spec.ts,settings.service.spec.tsusingTestBed.inject) sonpm test -- --watch=false --browsers=ChromeHeadlessruns 7 green tests. No karma.conf changes were needed (Wave 1 already updated karma/jasmine).ng add @angular-eslint/schematics(flateslint.config.js,@angular-eslint/builder:linttarget in angular.json), removedtslint+tslint.json. Rules for later waves are disabled rather than mass-rewriting code:prefer-standalone,prefer-inject,template/prefer-control-flow, and template a11y click/focus rules (Wave 2/3 territory);no-empty-functionallows empty DI constructors. Real issues were fixed instead of suppressed:let→const, unusederrorcallback params, unusedObservableimport,(data as any)→data['feedType'],declare let ga: Function→ typed fn, removed emptyngOnInits. One inline disable keeps the legacyitemselector (template change deferred).e2etarget,e2escript, protractor/jasminewd2/ts-node/jasmine-spec-reporter deps).package-lock.json; setcli.packageManager: npmin angular.json (staleyarn.lockotherwise breaksng add).Verified locally:
npm run build,npm test -- --watch=false --browsers=ChromeHeadless, andnpm run lintall pass.Link to Devin session: https://app.devin.ai/sessions/b8f234ccdeae49be8179afe0b48b633c
Requested by: @tobydrinkall
Devin Review