Fix Astro.logger producing no output in dev with the Cloudflare adapter - #17853
Fix Astro.logger producing no output in dev with the Cloudflare adapter#17853wakqasahmed wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 11e1396 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 |
github-actions
Bot
added
the
pkg: astro
Merging this PR will degrade performance by 15.55%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | Build: hybrid site (static + server) |
1.4 s | 1.7 s | -15.55% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing wakqasahmed:fix/issue-17823-logger-cloudflare (11e1396) with main (f8e9458)
| export const createApp: CreateApp = ({ streaming } = {}) => { | ||
| // Composition order: logger → environment → facade ctor | ||
| // (which warms the route table) → fetch handler → HMR wiring. | ||
| setLogger(manifest, createConsoleLogger(manifest.logLevel)); |
There was a problem hiding this comment.
I'm surprised this didn't throw a typescript error
There was a problem hiding this comment.
Good catch, and the answer turned out to be a real hole rather than a loose signature.
createConsoleLogger is properly typed — packages/astro/src/core/logger/impls/console.ts:38 declares createConsoleLogger({ level }: { level: AstroLoggerLevel }). The problem was on the other side: manifest was any at this call site, so nothing was being compared.
It came from packages/astro/dev-only.d.ts:
declare module 'virtual:astro:manifest' {
import type { SSRManifest } from './src/index.js';
export const manifest: SSRManifest;
}A top-level relative import inside an ambient declare module block is not legal — TS reports TS2439: Import or export declaration in an ambient module declaration cannot reference module through relative module name plus TS2307 on ./src/index.js. Because configs/tsconfig.base.json sets skipLibCheck: true, both errors are swallowed (they live in a .d.ts), SSRManifest degrades to any, and so does the exported manifest. You can see it directly — with the original declaration, const x: number = manifest.logLevel type-checks fine inside src/, while importing SSRManifest from ../../../../index.js in the same file correctly errors with Type 'string' is not assignable to type 'number'.
I've switched that one declaration to the inline import(...) type form, which is valid in an ambient module:
export const manifest: import('./src/index.js').SSRManifest;With that in place, tsc -b on the original code now fails exactly where you'd expect:
src/core/app/entrypoints/virtual/dev.ts(24,42): error TS2345: Argument of type 'string' is not assignable to parameter of type '{ level: AstroLoggerLevel; }'.
and passes with the fix. So the bug is now guarded by the compiler rather than only by a test.
One thing worth flagging separately: eight other declarations in dev-only.d.ts have the same problem (actions/entrypoint, routes, renderers, middleware, session-driver, dev-css, dev-css-all, component-metadata), and virtual:astro:adapter-entrypoint has export default any; which is TS2693. Converting all of them surfaces further pre-existing type errors — virtual:astro:routes is declared as RoutesList[] but both dev.ts and createAstroServerApp.ts consume it as RouteInfo[], and ImportedDevStyles no longer exists in src/types/astro.ts. That felt out of scope for a logger fix, so I left it alone; happy to open a separate issue or PR for it if you'd like.
There was a problem hiding this comment.
You're trying to make a test unit when it's supposed to be integration. Please remove those stubs and redesign the test
There was a problem hiding this comment.
Agreed, and you were right that the stubs were papering over the thing that actually needed testing. I've deleted packages/astro/test/units/logger/dev-entrypoint.test.ts along with the whole stubs/ directory and the registerHooks resolver, and replaced it with a real integration test.
The new test is packages/integrations/cloudflare/test/dev-logger.test.ts, with a fixture at test/fixtures/dev-logger/. That's the only place in the repo where the non-runnable dev entrypoint is genuinely exercised — vite-plugin-app resolves virtual:astro:app to astro/app/entrypoint/dev when command === 'serve', and the Cloudflare adapter is what puts a real worker in front of it. It follows the pattern of the existing dev-server tests there (dev-image-endpoint.test.ts, astro-dev-platform.test.ts): loadFixture plus startDevServer, then a real fetch. Nothing is mocked, stubbed, or resolver-patched.
The assertion is on real output rather than on internal state. The console logger writes inside the worker, whose stdout is not the test process's stdout, so the fixture page captures its own console.info/console.error around two Astro.logger calls and renders what was written; the test reads that back out of the HTML. One detail worth noting in case it looks odd: the fixture is loaded with logLevel: 'info', because loadFixture defaults every fixture to 'silent', which would have suppressed the records for an entirely legitimate reason and made the test green regardless.
I verified it fails for the right reason. With createConsoleLogger(manifest.logLevel) restored and the fixture's Vite dep cache cleared, the test fails with Expected two log records, got [] — matching the reported symptom, since isLogLevelEnabled(undefined, 'info') evaluates undefined <= 30 and drops every record. With the fix, both records come through.
The unit test that remains is the compile-time one described in the other thread: with the virtual:astro:manifest declaration corrected, tsc -b now rejects the original call outright.
…e test to an integration test (withastro#17823)
github-actions
Bot
added
the
pkg: integration
2 participants
Changes
Fixes #17823.
createConsoleLoggertakes an options object ({ level }), but the non-runnable dev entrypoint was calling it with the bare level string:Destructuring
{ level }out of a string yieldsundefined, so the logger was constructed with no level. Every call then goes throughisLogLevelEnabled(undefined, level), which evaluateslevels[undefined] <= levels[level]—undefined <= 30isfalse— so every message is dropped before it ever reaches the destination. That is whyAstro.logger/context.loggerare completely silent whileconsole.logstill shows up.This only affects adapters whose dev server runs in a non-runnable environment (workerd, i.e.
@astrojs/cloudflare), because that path loadsastro/app/entrypoint/dev(core/app/entrypoints/virtual/dev.ts). The Node adapter's dev server goes throughvite-plugin-app/createAstroServerApp.ts, which injects an already-constructed logger, so it was never affected — matching the report.The fix is to pass the options object the function actually expects.
Testing
Added
packages/astro/test/units/logger/dev-entrypoint.test.ts, which stubs thevirtual:astro:manifest/virtual:astro:fetchablemodules viaregisterHooks, callscreateApp()from the dev entrypoint, and asserts the manifest's logger reports the configured level and actually emits botherrorandinforecords.logger.level()isundefinedand nothing is written) and passes with the fix.packages/astrologger unit tests: 88 passing, 0 failing.pnpm --filter astro test:unit: 3360 tests, 3359 passing, 0 failing.tsc -bonpackages/astrois clean.Docs
No docs change needed — this restores documented
Astro.loggerbehaviour rather than changing it.