Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
69 changes: 5 additions & 64 deletions package-lock.json

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

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
"@angular/router": "~20.3.25",
"@angular/service-worker": "~20.3.25",
"node-fetch": "^2.6.0",
"rxjs": "~6.5.4",
"rxjs": "^7.8.0",
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.

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

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

"tslib": "^2.6.0",
"unfetch": "^4.1.0",
"zone.js": "~0.15.0"
Expand Down
11 changes: 5 additions & 6 deletions src/app/feeds/feed/feed.component.ts

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

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.

Correct — single-argument subscribe(callback) is not deprecated in RxJS 7, so app.component.ts was intentionally left as-is.

Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { Component, OnInit } from '@angular/core';
import { Observable } from 'rxjs';
import { Subscription } from 'rxjs';
import { ActivatedRoute } from '@angular/router';

Expand Down Expand Up @@ -37,14 +36,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 +42 to 45

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

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.

Correct — complete not firing on error is pre-existing behavior, and the *ngIf="items" guard prevents rendering stale listStart. No change needed here.

);
});
});
}
}
7 changes: 4 additions & 3 deletions src/app/item-details/item-details.component.ts
Original file line number Diff line number Diff line change
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
9 changes: 6 additions & 3 deletions src/app/shared/services/hackernews-api.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: () => { /* poll option fetch failed; leave partial data */ }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

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

});
}
}
Expand Down
7 changes: 4 additions & 3 deletions src/app/user/user.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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