Refactor internal request handling into a functional core - #17636
Open
matthewp wants to merge 13 commits into
Open
Refactor internal request handling into a functional core#17636matthewp wants to merge 13 commits into
matthewp wants to merge 13 commits into
Conversation
Replace the App/Pipeline god objects with pure functions drawing static
data from the manifest (the only permitted ambient source of truth):
- All internal handler classes (AstroHandler, PagesHandler, AstroMiddleware,
CacheHandler, ActionHandler, I18n, Rewrites, TrailingSlashHandler, error
handlers, session provider) are now module functions rooted in FetchState.
- FetchState constructs from (manifest, request, options?, hooks?); the
public one-arg new FetchState(request) works from a bare Request by
reaching the manifest module directly (ambient), with no app handle.
- Symbol.for('astro.app') and Symbol.for('astro.pipeline') are deleted;
nothing static rides the request. Render options carry only render()
inputs.
- Environment differences (prod SSR, dev runnable/non-runnable, build,
container) are per-manifest RenderEnvironment records composed at
entrypoint time; production is the zero-setup default.
- Process-lifetime derivations (route table, middleware, actions, session
driver, cache provider, logger, renderers) are WeakMap memos keyed by
the manifest in their owning modules.
- App and NodeApp remain public facades with unchanged signatures; every
method delegates to the functional core. app.pipeline survives as a
stateless compat shim. Pipeline base and all subclasses are deleted.
- Dev HMR route updates are now atomic (fixes stale-router split-brain).
No public API changes. All suites green: unit, full integration,
@astrojs/node, @astrojs/cloudflare (workerd), dev/HMR.
🦋 Changeset detectedLatest commit: e4ce203 The changes in this PR will be included in the next version bump. This PR includes changesets to release 419 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 |
matthewp
commented
Aug 10, 2026
matthewp
commented
Aug 10, 2026
matthewp
commented
Aug 10, 2026
matthewp
commented
Aug 10, 2026
matthewp
commented
Aug 10, 2026
Rewrites the three changesets to describe user-facing behavior instead of internals, removes all references to internal planning documents from code comments, and rewrites comments that described pre-refactor behavior to describe how the code currently works.
Covers the documented advanced pattern from #17591: a custom wrangler entryfile that builds its own state with `new FetchState(request)` from a bare workerd request, serves assets through `cf()`, and renders with `astro(state)`.
The method had no callers — BaseApp.render constructs the FetchState itself and generated builds only use the fetch member. Also rewrites the constructor comment to describe the current contract.
# Conflicts: # pnpm-lock.yaml
matthewp
marked this pull request as ready for review
August 10, 2026 21:51
Member
|
One thing I wanted to do is access the logger outside the request (and the pipeline), for example within |
There was a problem hiding this comment.
This is a large but well-scoped internal refactor that replaces the monolithic Pipeline and App class hierarchy with a manifest-keyed functional core. The change correctly preserves external API behavior while enabling new FetchState(request) to work inside custom worker entrypoints.
Overall assessment: The implementation is careful and thorough. State is correctly partitioned into per-request (FetchState), per-manifest (createManifestMemo), and per-environment (RenderEnvironment) layers. Backward-compatibility shims (AppPipeline) cover all previously exposed surface area. New unit tests and a Cloudflare integration regression test cover the motivating use case and the new memoization/registry primitives.
Specific notes:
- The functional error dispatch (
renderErrorPage→renderDefaultError/renderDevError/renderBuildError) preserves the old strategy selection. - The
AppPipelineshim reproduces every oldPipelinemember that was reachable fromapp.pipeline, with identical semantics. - Request-reconstruction during rewrites and forwarded-header application correctly drops the removed
appSymbol/pipelineSymbolbecause request-bound state is now captured onFetchStateitself. - Dev HMR wiring now targets the single per-manifest route table atomically, which fixes the stale-route issue described in one of the changesets.
One actionable issue found: BuildApp inadvertently passes streaming = true into the AppPipeline shim instead of the old manifest.serverLike value, making the backward-compatibility app.pipeline.streaming property incorrect for SSG builds.
This review was made by an LLM. The analysis may be wrong, and reports might be incorrect.
Contributor
Author
|
@florian-lefebvre yes, we can now get a logger keyed by the manifest. I can create a helper for this like we have for actions and some of the other things that hang off of the manifest. |
The last surviving Pipeline: app.pipeline was undocumented, so the stateless compat shim is gone entirely. The node adapter uses the new public app.getLogger() to wait for the configured log destination, and BuildApp carries the routeCache/getComponentByRoute accessors StaticPaths needs across the prerender bundle boundary. The internal PipelineFeatures bit flags are renamed to FetchFeatures, and @astrojs/node now requires the astro version that ships app.getLogger().
Replaces resolveLoggerDestination's hand-rolled WeakMap with a getResolvedLogger accessor built on the shared async manifest memo. The derivation never rejects: a custom log destination that fails to load is reported through the console logger and the request proceeds, instead of failing the first request.
Member
|
Amazing thanks! |
There was a problem hiding this comment.
Reviewed the functional request-core refactor from 630b833 through 8a164e6. This is a large, mostly mechanical refactor that removes the Pipeline/AppPipeline classes in favor of manifest-keyed functional helpers (RenderEnvironment, manifest memos, and the ambient manifest). It preserves the public App and astro/fetch APIs, adds a regression test for bare new FetchState(request) usage in the Cloudflare custom-worker pattern, and includes changesets for both astro and @astrojs/node.
No correctness or security blockers were found. Two optional items are noted: a missing error handler on the new dev-server body-stream collection that can leave a request hanging, and a small type-contract mismatch in the dev facade override of devMatch. The five changesets cover the user-visible facets of the refactor.
This review was made by an LLM. The analysis may be wrong, and reports might be incorrect.
Reject the dev body-buffering promise on stream error so an aborted or malformed upload surfaces through runWithErrorHandling instead of hanging the request, and make DevFacadeApp.devMatch's pathname optional to match the BaseApp contract.
There was a problem hiding this comment.
This is a large internal refactor that replaces the Pipeline/AppPipeline classes with a manifest-keyed functional core. The reviewed diff looks architecturally sound: per-manifest WeakMaps carry derived state, the ambient-manifest subpath allows new FetchState(request) to work without an app handle, and the environment record cleanly separates dev/build/container/production behavior. The test updates and the new Cloudflare custom-entryfile integration test cover the motivating issue. Changeset coverage is appropriate (astro and @astrojs/node). I found one packaging issue in the new imports mapping. No project code, tests, builds, or checks were run; the review was based entirely on the provided PR diff and repository reads.
This review was made by an LLM. The analysis may be wrong, and reports might be incorrect.
The #astro-internal/ambient-manifest types condition points at the src stub, which was not in the files list — nothing consults the path in the published package today, but shipping the file keeps the condition resolvable if a future declaration ever references the specifier.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
PipelineandAppclasses as used internally. These were essentially "god objects" that held state related to the server-side app.FetchStatewhen access from outside of theAppclass. For example in Cloudflare you can create a custom worker which is the entrypoint to the application.manifestis the one true god-object in SSR, and we could simply derive all state from that. So this new architecture is much more functional. Derived state is created ascreateManifestMemoandcreateAsyncManifestMemowhich are keyed on themanifest. Anything that needs this state can simply import it now.Appremains as its the external API for adapters, but mostly just defers to the functional approach now.Fixes #17591
Testing
Docs