Skip to content

Wave 2: Upgrade rxjs to ^7.8 and remove rxjs-compat#420

Open
tobydrinkall wants to merge 1 commit into
devin/1782947720-angular20-core-upgradefrom
devin/wave2-rxjs
Open

Wave 2: Upgrade rxjs to ^7.8 and remove rxjs-compat#420
tobydrinkall wants to merge 1 commit into
devin/1782947720-angular20-core-upgradefrom
devin/wave2-rxjs

Conversation

@tobydrinkall

@tobydrinkall tobydrinkall commented Jul 1, 2026

Copy link
Copy Markdown

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-compat removed.
  • Deep imports replaced with root imports:
    • rxjs/Observable and rxjs/operatorsimport { Observable, map } from 'rxjs' (hackernews-api.service.ts)
    • rxjs/Subscription'rxjs' (user.component.ts, item-details.component.ts)
  • Deprecated positional subscribe(next, error, complete) calls converted to observer-object form subscribe({ next, error, complete }) in feed.component.ts, item-details.component.ts, user.component.ts.
  • Typed lazyFetch(url: string, options?: RequestInit) to fix implicit-any fallout.

Verification

  • npm run build: passes (warnings only — Sass @import deprecations, budget/browserslist warnings).
  • npm run lint: fails with pre-existing Cannot 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 executes 0 of 0 specs (no spec files exist; Wave 4 adds tests).

Link to Devin session: https://app.devin.ai/sessions/28a497b9bf8548b19fd1af1471b5e203
Requested by: @tobydrinkall


Devin Review

Status Commit
⚪ Not started

Run Devin Review

Open in Devin Review (Staging)
Open in Devin Review

Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>
@tobydrinkall tobydrinkall self-assigned this Jul 1, 2026
@devin-ai-integration

Copy link
Copy Markdown

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Devin Review found 2 potential issues.

Open in Devin Review

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🚩 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)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 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)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pre-existing unused import, not touched by this PR — leaving it for the Wave 4 lint tooling pass to catch.

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.

1 participant