Repository navigation
Catch network errors in server island fetch to prevent uncaught TypeError - #18497
astro-factory[bot] wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 2012a86 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
astro-author owns this pull requestFactory's code author persona works on this branch when a maintainer or Factory's reviewer requests changes, or when checks fail: it pushes follow-up commits, replies to review threads, and asks for another review. It never merges. Unassign astro-author to take the pull request back; reassign it to start a fresh budget. Rounds used: 0 of 5 — astro-author (Factory persona) |
There was a problem hiding this comment.
Verdict: approved. No blocking findings remain.
GitHub doesn't let Factory approve a pull request it opened, so this verdict is recorded as a comment.
Findings
No findings.
Merge Requirement Assessment
Status: satisfied
- A rejected island
fetch()(GET or POST) must not cause an uncaughtTypeErrorand must keep the fallback (#18495): Both generated fetch calls inServerIslandComponent.getIslandContent()now end with.catch(() => {}), soresponsebecomesundefinedwhen the request fails.replaceServerIslandgains a!rguard, so it returns before touching the DOM. The new parameterized unit tests run the generated GET and POST scripts with a rejectingfetch. Without the fix, the top-levelawaitwould rejectrunIslandScript, so these tests would catch a regression. - A failed
r.text()read must keep the fallback: The body read is now wrapped intry { ... } catch { return; }before any DOM change. This matters becausereplaceServerIsland(...)isn't awaited in the island script, so a rejection there would otherwise surface as an unhandled promise rejection. The newrenderServerIslandRuntimetest covers it with aResponsewhosetext()rejects. The runtime's basic minification (trim lines, drop//lines, join with spaces) leaves the new multi-linetry/catchvalid, and the optional catch binding is ES2019 syntax, which the browsers that already run this module script support.
Review Status
Verdict: ready to merge based on static review
Context: PR #18497 plus linked issue #18495 (as described in the PR body)
Scope: PR base 5fe699c to head 2012a86 (3 files).
Changeset: present and covers astro (patch).
Limitations: The linked issue's body and comments were not retrieved separately; the requirements come from the PR description.
Validation: Static review only. No project code, tests, builds or checks were run.
GitHub context: collected
Git scope: PR base/head verified
This review was made by an LLM. The analysis may be wrong, and reports might be incorrect.
Merging this PR will not alter performance1 warning, 1 note
Comparing Footnotes |
Changes
fetch()calls in the generated server island client script no longer surface asUncaught (in promise) TypeError. Previously, when a fetch rejected (network drop, aborted navigation, blocked cross-origin redirect), the top-levelawaitin the island module would throw unhandled, polluting error trackers even though the fallback content remained visible. Both the GET and POST island fetch calls now append.catch(() => {}), andreplaceServerIslandreturns early on a missing response or a failedr.text()read, keeping the fallback silently.returnstatement suggested in the issue cannot be used at module top-level; the.catch(() => {})approach achieves the same effect without a syntax error.Testing
packages/astro/test/units/server-islands/server-islands-render.test.tsto cover the case wherefetchrejects, asserting that no error is thrown and the fallback content is preserved.Docs
Closes #18495