Skip to content

Catch network errors in server island fetch to prevent uncaught TypeError - #18497

Open
astro-factory[bot] wants to merge 1 commit into
mainfrom
factory/fix-18495
Open

astro-factory[bot] wants to merge 1 commit into
mainfrom
factory/fix-18495

Conversation

@astro-factory

@astro-factory astro-factory Bot commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

Changes

  • Failed fetch() calls in the generated server island client script no longer surface as Uncaught (in promise) TypeError. Previously, when a fetch rejected (network drop, aborted navigation, blocked cross-origin redirect), the top-level await in 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(() => {}), and replaceServerIsland returns early on a missing response or a failed r.text() read, keeping the fallback silently.
  • The return statement suggested in the issue cannot be used at module top-level; the .catch(() => {}) approach achieves the same effect without a syntax error.

Testing

  • Extended packages/astro/test/units/server-islands/server-islands-render.test.ts to cover the case where fetch rejects, asserting that no error is thrown and the fallback content is preserved.

Docs

  • No docs update needed; this is an internal error-handling fix with no new public API or configuration surface.

Closes #18495

@astro-factory astro-factory Bot added the fix verified Reporter confirmed the triage bot fix works label Oct 23, 2026
@changeset-bot

changeset-bot Bot commented Oct 23, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 2012a86

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
astro Patch

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

astro-factory Bot commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

astro-author owns this pull request

Factory'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
Status: waiting for feedback.

— astro-author (Factory persona)

@github-actions github-actions Bot added the pkg: astro Related to the core `astro` package (scope) label Oct 23, 2026
@astro-factory
astro-factory Bot removed the request for review from astro-reviewer October 23, 2026 14:22

@astro-factory astro-factory Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 uncaught TypeError and must keep the fallback (#18495): Both generated fetch calls in ServerIslandComponent.getIslandContent() now end with .catch(() => {}), so response becomes undefined when the request fails. replaceServerIsland gains a !r guard, so it returns before touching the DOM. The new parameterized unit tests run the generated GET and POST scripts with a rejecting fetch. Without the fix, the top-level await would reject runIslandScript, so these tests would catch a regression.
  • A failed r.text() read must keep the fallback: The body read is now wrapped in try { ... } catch { return; } before any DOM change. This matters because replaceServerIsland(...) isn't awaited in the island script, so a rejection there would otherwise surface as an unhandled promise rejection. The new renderServerIslandRuntime test covers it with a Response whose text() rejects. The runtime's basic minification (trim lines, drop // lines, join with spaces) leaves the new multi-line try/catch valid, 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.

@codspeed

codspeed Bot commented Oct 23, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

1 warning, 1 note

⚠️ 3 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

ℹ️ 1 benchmark has no flamegraph

The flamegraph wasn't generated: most of the time was spent in runtime internals, which would leave nothing useful to analyze.

See affected benchmarks

✅ 21 untouched benchmarks


Comparing factory/fix-18495 (2012a86) with main (d145d19)1

Open in CodSpeed

Footnotes

  1. No successful run was found on main (5fe699c) during the generation of this report, so d145d19 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix verified Reporter confirmed the triage bot fix works pkg: astro Related to the core `astro` package (scope)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Server island: uncaught TypeError when the island fetch fails

1 participant