Fix issue with dynamic routes in complex projects using workerd#16720
Fix issue with dynamic routes in complex projects using workerd#16720thomas-callahan-collibra wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: 1a86c67 The changes in this PR will be included in the next version bump. This PR includes changesets to release 421 packages
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 |
|
You burned tons of tokens to emit this dissertation for a mere 3 lines of code, and you didn't even have the decency to test the changes yourself (the app doesn't build). 🤦 |
|
A ton of tokens maybe, but also almost an entire working day trying to track this issue down on my own. Does it matter that it's 3 lines of code? It did build locally, but I dropped an export statement by accident when cleaning up. Sorry for the inconvenience. It's fixed now, the build has completed, waiting for tests to complete. This kind of response is why people hesitate to try to contribute to OS. I'm trying to help fix a bug in your project that has cost me considerable time and effort (on top of another bug I reported and helped test a week or two ago). I'm doing my best with a project whose code I don't know and a very complex build/CI setup. Your own project guidelines say:
If there's anything still not right with this PR, I'm happy to help further if you have any additional constructive feedback. |
|
Not all contributions are the same. Here's why I think this contribution triggered me on a personal level and I found it not respectful towards me:
I am not keen in accepting a fix if the contributor can't explain it. |
|
As I read it (and it was not that long of a read), "Claude fix" => here's what's happening does not mean he's not understanding it, on the contrary. It's clear he understands it and it's a legit bug. And instead of tangling up Enterprise support, he's contributing a fix for your benefit, the project, and its owner. And you talk of decency. I hate to quote that guy but did you even say thank you once? |
|
I'll also point out that every existing test passes, the only untested path is a project sufficiently complex to test the specific issue this fixes, which I can't imagine how you would do in a CI test runner. What are you going to do, set up a project with dozens of components and a bunch of imported libs just for a CI test?
This isn't some AI slop bug report and fix. If you're going to reject it because AI was involved then I don't know what to say. I couldn't have found it without it, and nobody else has apparently found this either. The issue does take a specific set of circumstances to trigger, but it is real. I opened this other issue a few weeks ago, your own project's AI bot suggested a fix, someone from your team had me test, I confirmed the fix, and the change was merged earlier this week. How is this any different except that I did the analysis on my own and contributed a change rather than just opening an issue? If there is serious doubt from the team that this is correct, then close this and I'll just open an issue while I continue using the post-install patch so I can get this project moving again. |
|
Regarding the issue
I believe we have encountered this issue a few months ago where all our tests started to fail. This was an issue on CF side. Once we they fixed it, everything was green. Is there a particular reason why you need to stick to this compatibility date? |
|
No, but the complex project that actually triggers this issue uses |
|
So that is interesting -- the problem does go away with For our projects, I can bump our compatibility dates (we're migrating from Astro 5->6 so they all have dates older than this), but I'm not sure everybody has that liberty. If that's the fix, it should probably be mentioned in the migration docs though, and maybe the CF adapter docs? I can redo this PR to drop the code change and change it to just a docs update if you want. I did do some more investigation though. I logged the values that are used by the Test results
I can see the difference that results in this problem -- when using a "bad" compatibility date without the patch applied, there are two instances where isNode is set to true even though it's running workerd. This is also true when the compatibilty date is "good" -- in fact it ALWAYS evaluates to true, but workerd seems to be able to handle being treated like Node now. With the patch applied, isNode is always accurate to the environment. This might no longer be necessary because of changes to workerd, but it is technically correct and works for all compatibility dates. But assuming you want to maintain compatibility with older workerd dates, updating the isNode check is still correct. In fact it probably isn't necessary to check the string value of
or just in case the string changes slightly in the future?
I wouldn't ordinarily rely on a userAgent string because of its history of unreliability in browsers, but in workerd I'd expect it to be stable and reliable. I don't love the double negative logic either, but flipping that would mean changing the variable name to |
Changes
Fixes dynamic routes returning the string
[object Object]under the following conditions:nodejs_compatflag. The affected projects all use compatibility dates of2025-04-01or newer, not sure if older dates are affected.This was a very difficult issue to track down. Static routes were fine, and in the Worker logs I could see that the dynamic routes were being processed correctly (I see things like logs indicating success for loading external content and other expected internal logging from the build process) but the returned response was just the string
[object Object]regardless of which page or what content was expected -- no error messages of any kind.I did not open an issue for this because by the time I had enough detail to do so, Claude Code had provided a fix, so I'm just going straight to a PR.
See included Claude Code analysis below for full details.
Testing
I did not add tests because I'm not sure how to provide a sufficiently complicated test project -- this problem only surfaces in larger projects. I'm happy to assist adding tests if I can get some help.
I do have this fix running in 3 separate projects ranging in size and complexity from a stripped down copy of the Astro blog starter template for Workers (where the problem never exhibited but this fix does not cause any issues), up to a 1000-page hybrid site with upwards of 100 Astro components and a mix of dynamic and static routes (where the problem surfaced and the fix works).
Claude Code's explanation of the issue and proposed fix were arrived at after several rounds of analysis and refinements, and its explanation makes sense and matches what I've been experiencing. I specifically asked it to check that the changes do not affect anything for Deno or Node prerendering environments, only workerd.
Docs
I don't believe docs updates are needed because this fixes an issue with expected functionality with no changes needed by the user.
Claude's analysis, RCA, and proposed fix, which I've implemented in this PR
If you're interested, here's what my AI agent found
This analysis was done with Astro 6.2.x and Cloudflare 13.3.x, but I've confirmed the problem still exists in the current versions. I'm barred by work policy from installing packages newer than 7 days so I cannot test the latest versions directly, but the problem code still exists in the latest version, unchanged.This analysis was done by comparing two projects:
astro-blog-starter-templatewhich is a copy of the Astro starter for Workers, stripped of all but one dynamic and one static test pageweb-dev-collibra-astrowhich is our largest, most complicated project where the problem surfacedThis is its final summary, verbatim:
Fix Summary — Astro
isNodefalse-positive in workerdSymptom
In an Astro 6.2.x project deployed to Cloudflare Workers via
@astrojs/cloudflare13.3.x withnodejs_compat, any route withprerender: falsereturns anHTTP 200,content-type: text/htmlresponse whose body is the 15-byte string[object Object]. Static / prerendered routes are unaffected. Reproducible locally withastro build && astro preview.Root cause
astro/dist/runtime/server/render/util.jsdefines:astro/dist/runtime/server/render/page.jsthen chooses the response body shape:With
nodejs_compat, workerd'sprocessreportsObject.prototype.toString.call(process) === "[object process]"until the@cloudflare/unenv-presetpolyfill replaces it with a plain object. The output ofrenderToAsyncIterableis an object exposingSymbol.asyncIterator. Node'sundiciaccepts async iterables as a Response body; workerd'sResponsedoes not, and falls back toString(body)→"[object Object]".Why the bug surfaces in some projects but not others
The bug is a module-initialization-order race that only manifests once Rollup decides to code-split Astro's shared runtime.
Both the failing project and the working starter contain the same two pieces of code in the worker bundle:
The order in which A and B run depends on whether Rollup keeps them in one chunk or splits them across two:
astro-blog-starter-template(2 pages, almost no Astro internals reused) — Rollup keeps everything in one chunk,worker-entry_DEEjL72T.mjs:Single module body, top-down execution →
processis the plain-object unenv polyfill by the timeisNoderuns →isNode = false→renderToReadableStream→ works.web-dev-collibra-astro(many pages and components importing fromastro/runtime/server/*) — Rollup factors Astro's shared runtime into its own chunk:worker-entryimportstransition. ES Module spec executes imported module bodies before the importer's body, so B runs before A.isNodeis evaluated against workerd's rawnodejs_compatprocess, latches totrue, andrenderToAsyncIterableis selected → broken response.The threshold is whatever pushes Rollup over its code-splitting heuristic for shared dependencies. Any project with enough reuse of
astro/runtime/server/*across pages or components will eventually trip it — it's not specific to Vue, custom integrations, or middleware.I confirmed module init order is the differentiator by logging
Object.prototype.toString.call(process)at module init vs. request time in the failing project:"[object process]"at init,"[object Object]"at request time — proving the polyfill assignment happens between the two, afterisNodehas already been frozen.The fix
One line in
packages/astro/src/runtime/server/render/util.ts:Workerd's
navigator.userAgent === "Cloudflare-Workers"is documented, stable, and present from t=0 — so the check is immune to the module-init-order race that broke the originalprocess-only detection.Behavior matrix
processtoStringnavigator.userAgentisNodebeforeisNodeafter"[object process]"truetrue"[object process]"(Node compat shim)"Deno/x.y.z"truetrue!isDenoinrenderPage"Cloudflare-Workers"falsefalsenodejs_compat"Cloudflare-Workers"falsefalsenodejs_compat, polyfill already installed beforeisNodeeval"[object Object]""Cloudflare-Workers"falsefalsenodejs_compat, polyfill not yet installed"[object process]""Cloudflare-Workers"true(wrong)false(correct)The change only flips behavior in the row where the original heuristic is wrong.
Cross-checks against the prerender flow
@astrojs/cloudflareexposesprerenderEnvironment: "node" | "workerd"(default"workerd").prerenderEnvironment: "node": prerender runs in real Node. Nonavigatorglobal, originalisNode = truepreserved. Same as before. ✓prerenderEnvironment: "workerd"(default): prerender runs in workerd. New check correctly returnsfalse. TherenderToReadableStreampath produces aReadableStreambody, which Astro's prerender pipeline consumes the same way it consumes an async iterable. ✓Verification performed
web-dev-collibra-astrowithastro build+astro preview—curl /dev/dynamicreturns[object Object](15 bytes).isNode,streaming, andbody.constructor.nameinside the bundledrenderPage—isNode=true, body constructor isObject(the async iterable), notReadableStream.Object.prototype.toString.call(process)both at module init ("[object process]") and at request time ("[object Object]") — proves the polyfill assignment happens between the two.astro-blog-starter-templateevaluatesisNode = falsebecause the unenv polyfill assignment (line 558) precedes theisNodeevaluation (line 5864) in the same chunk.build-scripts/patch-astro-renderer.mjs; re-ran preview; dynamic route returns<!DOCTYPE html>...<div>TEST</div>, static route unchanged.Details