chore: upgrade RxJS to v7 and migrate deprecated APIs#427
Conversation
- Update rxjs from ~6.5.4 to ^7.8.0
- Replace deprecated positional subscribe(next, error, complete) with
observer object pattern subscribe({ next, error, complete })
- Remove unused Observable import from feed.component.ts
- All changes verified with ng build (zero TS errors)
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}
| "@angular/service-worker": "~20.3.25", | ||
| "node-fetch": "^2.6.0", | ||
| "rxjs": "~6.5.4", | ||
| "rxjs": "^7.8.0", |
There was a problem hiding this comment.
📝 Info: Broad version range for RxJS allows all future 7.x releases
The version specifier changed from ~6.5.4 (patch-only updates within 6.5.x) to ^7.8.0 (any 7.x.x >= 7.8.0). The tilde-to-caret change means future npm install runs could pull in minor version bumps (e.g., 7.9.0, 7.10.0) that might include behavioral changes. The previous convention in this project used ~ for rxjs. Consider whether ~7.8.0 would be more appropriate to match the conservative versioning style used for other dependencies like zone.js (~0.15.0) and the Angular packages (~20.3.25).
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — ^7.8.0 was specified in the task requirements. The caret range is intentional here to allow minor RxJS 7 updates (which follow semver), but the tradeoff is noted.
In RxJS 7, unhandled observable errors are reported via reportUnhandledError (async), making them uncatchable. Add an error handler to the poll content subscribe to prevent uncaught exceptions. Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>
There was a problem hiding this comment.
📝 Info: app.component.ts still uses single-callback subscribe form
The subscribe(callback) call in src/app/app.component.ts:25 uses the single-argument form which is NOT deprecated in RxJS 7 (only the multi-argument subscribe(next, error, complete) form is deprecated). So this file correctly does not need migration. However, it has no error handler for router events, which is consistent with the pre-existing pattern.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Correct — single-argument subscribe(callback) is not deprecated in RxJS 7, so app.component.ts was intentionally left as-is.
| story.poll[i - 1] = pollResults; | ||
| story.poll_votes_count += pollResults.points; | ||
| }, | ||
| error: () => { /* poll option fetch failed; leave partial data */ } |
There was a problem hiding this comment.
🚩 Silent error swallowing for poll fetches is a behavioral change
The old code at src/app/shared/services/hackernews-api.service.ts:29-35 had no error handler for fetchPollContent subscribes. In RxJS 6, an unhandled error in subscribe() would throw synchronously, potentially surfacing the issue. In RxJS 7, unhandled errors are reported asynchronously via config.onUnhandledError (and would log to console by default). The PR adds error: () => { /* poll option fetch failed; leave partial data */ } which silently swallows the error entirely — no console log, no user feedback. This means if poll option fetches fail, the user sees partial/stale poll data with no indication that something went wrong. This is a deliberate design choice per the comment, but worth confirming it's the desired UX.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged. The silent handler is intentional — poll options are supplementary data fetched inside a map operator, so there's no clean path to surface errors to the user from here. Graceful degradation (showing the story without poll breakdown) is preferable to an uncaught exception. A console.warn could be added if observability is desired, but that's outside the scope of this migration.
| complete: () => { | ||
| this.listStart = ((this.pageNum - 1) * 30) + 1; | ||
| window.scrollTo(0, 0); | ||
| } |
There was a problem hiding this comment.
📝 Info: Feed list numbering and scroll position are skipped on error, same as before
The complete callback at src/app/feeds/feed/feed.component.ts:42-45 sets listStart and calls window.scrollTo(0, 0). In both RxJS 6 and 7, complete does not fire when error fires. This means on a fetch error, listStart retains its previous value (or is undefined on first load). The template at src/app/feeds/feed/feed.component.html:10 uses start="{{ listStart }}" inside *ngIf="items", and since items won't be set on error either, the <ol> won't render. So this is safe — but it's worth noting that listStart could carry a stale value from a previous successful page load if the user navigates to a new page that errors.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Correct — complete not firing on error is pre-existing behavior, and the *ngIf="items" guard prevents rendering stale listStart. No change needed here.
1 participant
Summary
Wave 2 of the Angular upgrade: migrates RxJS from v6 to v7 on top of the Angular 20 foundation (Wave 1, #426).
RxJS 6 → 7 upgrade:
"rxjs": "~6.5.4"→"^7.8.0"inpackage.jsonDeprecated API migration — positional
subscribe(nextFn, errorFn, completeFn)replaced with observer objects:Affected files:
FeedComponent,ItemDetailsComponent,UserComponent,HackerNewsAPIService.Bug fix: Added missing error handler to
fetchPollContentsubscribe inHackerNewsAPIService. In RxJS 7, unhandled observable errors are reported viareportUnhandledError(async), making them uncatchable — this would cause uncaught exceptions on poll option fetch failures.Removed unused
Observableimport fromFeedComponent.No
toPromise()orrxjs/internal/*imports found. Build (ng build) passes with zero TS errors.Link to Devin session: https://app.devin.ai/sessions/1de0d85e7f064921b574a0a88d9b574d
Requested by: @tobydrinkall
Devin Review