Wave 2: Upgrade rxjs to ^7.8 and remove rxjs-compat#420
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:
|
There was a problem hiding this comment.
🚩 Poll content fetches lack error handling, now with different RxJS 7 unhandled-error semantics
The fetchPollContent subscribe at src/app/shared/services/hackernews-api.service.ts:28 uses .subscribe(pollResults => {...}) with no error handler. This is pre-existing, but the RxJS 6→7 upgrade changes how unhandled errors are reported: RxJS 7 uses reportUnhandledError (via setTimeout) instead of throwing synchronously. If a poll content fetch fails, the error will surface as an uncaught async error rather than being thrown on the call stack. This won't crash the app but changes error observability. Since this wasn't touched by the PR and the single-argument subscribe form is not deprecated, it's not a bug in the PR itself.
(Refers to lines 28-31)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — pre-existing behavior, and the single-argument subscribe(next) form remains supported in RxJS 7. Leaving as-is to keep this PR scoped to the RxJS 7 migration.
There was a problem hiding this comment.
📝 Info: Unused Observable import retained in feed component
Line 2 imports Observable from rxjs, but it is never used anywhere in the FeedComponent class. This was also unused before the PR (imported from rxjs previously). A minor cleanup opportunity — lint rules should catch this.
(Refers to line 2)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Pre-existing unused import, not touched by this PR — leaving it for the Wave 4 lint tooling pass to catch.
Summary
Wave 2 of the Angular 9→20 upgrade (based on #417's branch): migrates the app off rxjs 6 + rxjs-compat to rxjs ^7.8.
package.json:rxjs~6.6.7→^7.8.2,rxjs-compatremoved.rxjs/Observableandrxjs/operators→import { Observable, map } from 'rxjs'(hackernews-api.service.ts)rxjs/Subscription→'rxjs'(user.component.ts,item-details.component.ts)subscribe(next, error, complete)calls converted to observer-object formsubscribe({ next, error, complete })infeed.component.ts,item-details.component.ts,user.component.ts.lazyFetch(url: string, options?: RequestInit)to fix implicit-any fallout.Verification
npm run build: passes (warnings only — Sass@importdeprecations, budget/browserslist warnings).npm run lint: fails with pre-existingCannot find builder "@angular-devkit/build-angular:tslint"— lint tooling removal is being addressed in Wave 4, not touched here.npm test: runs and exits green but executes0 of 0specs (no spec files exist; Wave 4 adds tests).Link to Devin session: https://app.devin.ai/sessions/28a497b9bf8548b19fd1af1471b5e203
Requested by: @tobydrinkall
Devin Review