Skip to content

Wave 2: Upgrade to RxJS 7 and remove rxjs-compat #422

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tobydrinkall wants to merge 3 commits into
base: devin/1782984123-angular20-core-upgrade
Choose a base branch
from
Open

Wave 2: Upgrade to RxJS 7 and remove rxjs-compat #422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5834a7b
Upgrade to RxJS 7, remove rxjs-compat, replace deep rxjs imports
devin-ai-integration[bot] Jul 2, 2026
687be87
Use observer-object subscribe signature (RxJS 7 idiomatic form)
devin-ai-integration[bot] Jul 2, 2026
f117d91
Add error handler to poll content subscribe
devin-ai-integration[bot] Jul 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 5 additions & 71 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 1 addition & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@
"@angular/router": "^21.0.0",
"@angular/service-worker": "^21.0.0",
"node-fetch": "^2.6.0",
"rxjs": "~6.5.4",
"rxjs-compat": "^6.5.2",
"rxjs": "^7.8.2",
"tslib": "^2.6.0",
"unfetch": "^4.1.0",
"zone.js": "~0.16.0"
Expand Down
10 changes: 5 additions & 5 deletions src/app/feeds/feed/feed.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,14 @@ export class FeedComponent implements OnInit {
this.pageSub = this.route.params.subscribe(params => {
this.pageNum = params['page'] ? +params['page'] : 1;
this._hackerNewsAPIService.fetchFeed(this.feedType, this.pageNum)
.subscribe(
items => this.items = items,
error => this.errorMessage = 'Could not load ' + this.feedType + ' stories.',
() => {
.subscribe({
next: items => this.items = items,
error: () => this.errorMessage = 'Could not load ' + this.feedType + ' stories.',
complete: () => {
this.listStart = ((this.pageNum - 1) * 30) + 1;
window.scrollTo(0, 0);
}
Comment on lines +43 to 46

@devin-ai-integration devin-ai-integration Bot Jul 2, 2026

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: 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.

Open in Devin Review

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

);
});
});
}
}
9 changes: 5 additions & 4 deletions src/app/item-details/item-details.component.ts
View file
Open in desktop
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Component, OnInit } from '@angular/core';
import { ActivatedRoute } from '@angular/router';
import { Location } from '@angular/common';
import { Subscription } from 'rxjs/Subscription';
import { Subscription } from 'rxjs';

import { HackerNewsAPIService } from '../shared/services/hackernews-api.service';
import { SettingsService } from '../shared/services/settings.service';
Expand Down Expand Up @@ -33,9 +33,10 @@ export class ItemDetailsComponent implements OnInit {
ngOnInit() {
this.sub = this.route.params.subscribe(params => {
let itemID = +params['id'];
this._hackerNewsAPIService.fetchItemContent(itemID).subscribe(item => {
this.item = item;
}, error => this.errorMessage = 'Could not load item comments.');
this._hackerNewsAPIService.fetchItemContent(itemID).subscribe({
next: item => this.item = item,
error: () => this.errorMessage = 'Could not load item comments.'
});
});
window.scrollTo(0, 0);
}
Expand Down
11 changes: 7 additions & 4 deletions src/app/shared/services/hackernews-api.service.ts
View file
Open in desktop

@devin-ai-integration devin-ai-integration Bot Jul 2, 2026 •
edited
Loading

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: 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.

Open in Devin Review

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

@devin-ai-integration devin-ai-integration Bot Jul 2, 2026

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 — 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.

Comment thread
devin-ai-integration[bot] marked this conversation as resolved.

@devin-ai-integration devin-ai-integration Bot Jul 2, 2026

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

Open in Devin Review

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

@devin-ai-integration devin-ai-integration Bot Jul 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

@devin-ai-integration devin-ai-integration Bot Jul 2, 2026

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

Open in Devin Review

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

Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Injectable } from '@angular/core';
import { Observable } from 'rxjs/Observable';
import { Observable } from 'rxjs';
import fetch from 'unfetch';
import {map } from 'rxjs/operators';

Expand All @@ -26,9 +26,12 @@ export class HackerNewsAPIService {
let numberOfPollOptions = story.poll.length;
story.poll_votes_count = 0;
for (let i = 1; i <= numberOfPollOptions; i++) {
this.fetchPollContent(story.id + i).subscribe(pollResults => {
story.poll[i - 1] = pollResults;
story.poll_votes_count += pollResults.points;
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)
});
Comment on lines +29 to 35

@devin-ai-integration devin-ai-integration Bot Jul 2, 2026

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: 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.

Open in Devin Review

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

}
}
Expand Down
9 changes: 5 additions & 4 deletions src/app/user/user.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Component, OnInit } from '@angular/core';
import { ActivatedRoute } from '@angular/router';
import { Location } from '@angular/common';
import { Subscription } from 'rxjs/Subscription';
import { Subscription } from 'rxjs';

import { HackerNewsAPIService } from '../shared/services/hackernews-api.service';
import { User } from '../shared/models/user';
Expand All @@ -26,9 +26,10 @@ export class UserComponent implements OnInit {
ngOnInit() {
this.sub = this.route.params.subscribe(params => {
let userID = params['id'];
this._hackerNewsAPIService.fetchUser(userID).subscribe(data => {
this.user = data;
}, error => this.errorMessage = 'Could not load user ' + userID + '.');
this._hackerNewsAPIService.fetchUser(userID).subscribe({
next: data => this.user = data,
error: () => this.errorMessage = 'Could not load user ' + userID + '.'
});
});
}

Expand Down