Wave 2: Upgrade to RxJS 7 and remove rxjs-compat#422
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}
There was a problem hiding this comment.
📝 Info: Subtle RxJS 7 behavioral change in lazyFetch error-after-complete scenario
In lazyFetch at src/app/shared/services/hackernews-api.service.ts:48-65, if fetchObserver.next(data) at line 56 throws (e.g., due to an error in a downstream operator), the promise .catch at line 60 would fire and call fetchObserver.error(err) on an already-completed/errored subscriber. In RxJS 6, this was silently ignored. In RxJS 7, this reports to config.onUnhandledError, which by default logs a console warning. This is unlikely to cause user-visible issues but could produce unexpected console noise if downstream operators throw.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — this is an informational behavioral note (worst case a console warning via config.onUnhandledError). Leaving lazyFetch as-is for this wave; no user-visible impact expected.
Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>
There was a problem hiding this comment.
📝 Info: Nested subscribe inside map operator is a pre-existing anti-pattern that could cause race conditions with polls
The fetchItemContent method at src/app/shared/services/hackernews-api.service.ts:24-36 subscribes to fetchPollContent inside a map operator. This means the outer observable completes and emits the story object before the inner poll fetches resolve. The subscriber at src/app/item-details/item-details.component.ts:36-39 receives a story with placeholder poll data that gets mutated asynchronously later. This is a pre-existing pattern, but the RxJS 7 migration is a good opportunity to note it — idiomatic RxJS would use switchMap/forkJoin to compose these properly.
(Refers to lines 23-37)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Agreed this nested-subscribe-in-map pattern is a pre-existing anti-pattern; a switchMap/forkJoin refactor would change composition/behavior and is out of scope for this wave, so leaving as-is.
Co-Authored-By: Toby Drinkall <toby.drinkall@cognition.ai>
There was a problem hiding this comment.
📝 Info: Custom Observable wrapper in lazyFetch is compatible with RxJS 7 but has a pre-existing teardown gap
The lazyFetch function at src/app/shared/services/hackernews-api.service.ts:51-68 creates a raw Observable that wraps fetch. When the subscriber unsubscribes (setting cancelToken = true), the in-flight fetch promise is not actually aborted — only the next/complete calls are suppressed. The observer is never completed or errored in this case, which means teardown finalizers that depend on completion won't run. This is pre-existing behavior unchanged by this PR, but RxJS 7's Subscriber has slightly different internal finalization logic. In practice this is unlikely to cause issues here since the teardown function itself is returned, but it's worth being aware of if this pattern is extended.
(Refers to lines 51-68)
Was this helpful? React with 👍 or 👎 to provide feedback.
| this.fetchPollContent(story.id + i).subscribe({ | ||
| next: pollResults => { | ||
| story.poll[i - 1] = pollResults; | ||
| story.poll_votes_count += pollResults.points; | ||
| }, | ||
| error: err => console.error('Could not load poll option', story.id + i, err) | ||
| }); |
There was a problem hiding this comment.
📝 Info: New poll error handler prevents unhandled errors in RxJS 7
The old code at src/app/shared/services/hackernews-api.service.ts:29-35 had no error handler for fetchPollContent subscriptions. In RxJS 6, an unhandled observable error would throw synchronously via hostReportError. In RxJS 7, unhandled errors are thrown asynchronously via reportUnhandledError (using setTimeout), which makes them harder to debug and can trigger global error handlers. The newly added error handler (error: err => console.error(...)) is a necessary addition for the RxJS 7 upgrade, not just a nice-to-have.
Was this helpful? React with 👍 or 👎 to provide feedback.
| complete: () => { | ||
| this.listStart = ((this.pageNum - 1) * 30) + 1; | ||
| window.scrollTo(0, 0); | ||
| } |
There was a problem hiding this comment.
📝 Info: Feed list position and scroll only update on success, not on error
The complete callback at lines 43-46 sets listStart and calls window.scrollTo(0, 0). Since RxJS observables do not call complete after error, these side effects are skipped when the feed fetch fails. This was the same behavior before the migration (the third positional argument to subscribe was the complete handler), but it's worth noting: on error, listStart retains its previous value and the page doesn't scroll to top. If the user navigates from page 2 (where listStart=31) and the next page errors, the stale listStart could cause a confusing numbered list if items are later displayed.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Wave 2 of the Angular 9→21 migration: RxJS modernization on top of the Wave 1 core-upgrade branch.
rxjs~6.5.4→^7.8.2; droppedrxjs-compatentirely (package.json + regenerated package-lock.json)import { Observable } from 'rxjs/Observable'→from 'rxjs'(hackernews-api.service.ts)import { Subscription } from 'rxjs/Subscription'→from 'rxjs'(item-details.component.ts, user.component.ts)subscribe(next, error, complete)calls to the observer-object formsubscribe({ next, error, complete })(item-details, user, feed components)errorhandler to thefetchPollContentsubscription infetchItemContent— in RxJS 7 unhandled subscription errors are reported asynchronously instead of rethrown, so poll fetch failures would otherwise be silently swallowednpm run buildcompletes with zero errors andrg "rxjs-compat|rxjs/Observable|rxjs/Subscription"returns nothing.Link to Devin session: https://app.devin.ai/sessions/d94fd6cfc50b45acbe137ebc4cd2d0a6
Requested by: @tobydrinkall
Devin Review