Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: pipeline lifetime #9795

Merged
merged 20 commits into from Feb 20, 2024
Merged

Conversation

lilnasy
Copy link
Contributor

@lilnasy lilnasy commented Jan 23, 2024

Changes

  • A Pipeline lasted the entire duration of a server while being concerned with only the request.
  • This resulted in unsafe mutations needing to be performed on the global Pipeline before serving a request.
  • The concept of hooks had to be introduced to make per-request actions easier.
  • This PR repositions RenderContext as the hub of rendering by bringing control over creating other downstream objects, and rendering to it.
  • Data previously in Environment is rolled into Pipeline, which now also exposes functions to retrieve styles and scripts for RenderContext to use.
  • RenderContext is now in a sturdier position to take on the complexity of shifting RouteData during the course of middleware chain execution.
  • See the new render-context.ts
                                                                                 
                                                                                               -->-- SSRResult -->-- Astro Global
                                                                             Request          /
                           App --- AppPipeline ---                              |            /
                                                  \                             v           /
       vite-plugin-astro-server --- DevPipeline --->--- Base Pipeline --- RenderContext ----                          -->-- Middleware
                                                  /    (per process)        (per page)      \                        /
                AstroBuilder --- BuildPipeline ---                                           \                      /
                                                                                              \                    /
                                                                                               -->-- APIContext -->-- Endpoint

(proposed)
  • Each commit can be reviewed individually and passes all tests.

Testing

  • Adjusted unit tests. Existing tests should pass.

Future

  • routeData.type could go away. Differences between endpoint, pages, redirects, and fallbacks could be handled by the compiled page chunks themselves. It would be easy to create a standard ssrEntry format that exports a RenderContext -> Response function.
  • It would greatly reduce the minimum server bundle size, because renderPage would not have to be included if all pages are prerendered and only ssrEntrys for endpoints remain.
  • This PR defers gathering scripts, styles, links, propagation metadata to the "last" next(), giving the middleware chain the opportunity to change routeData, and "reroute" the request.

Copy link

changeset-bot bot commented Jan 23, 2024

🦋 Changeset detected

Latest commit: a6cf337

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

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 github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Jan 23, 2024
@lilnasy lilnasy marked this pull request as ready for review January 28, 2024 18:45
@ematipico ematipico self-requested a review January 30, 2024 08:49
@lilnasy lilnasy force-pushed the refactor-pipeline-lifetime branch 3 times, most recently from 1ddd87d to 7763d76 Compare January 31, 2024 22:25
packages/astro/src/core/render/environment.ts Outdated Show resolved Hide resolved
packages/astro/src/core/render/environment.ts Outdated Show resolved Hide resolved
* An environment represents the static parts of rendering that do not change
* between requests. These are mostly known when the server first starts up and do not change.
* Thus, they can be created once and passed through to renderPage on each request.
* The environment represents the static parts of rendering that do not change between requests.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* The environment represents the static parts of rendering that do not change between requests.
* The environment represents the static parts of rendering that do not change among requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original seems correct.

packages/astro/src/core/render/environment.ts Outdated Show resolved Hide resolved
super(logger, manifest, mode, [], resolve, serverLike, streaming, routeCache);
}

async preload(filePath: URL) {
Copy link
Member

Choose a reason for hiding this comment

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

Docs missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not new code. It is brought in from https://github.com/withastro/astro/blob/astro@4.3.5/packages/astro/src/vite-plugin-astro-server/index.ts#L7. I don't have the context to document it.

packages/astro/src/vite-plugin-astro-server/route.ts Outdated Show resolved Hide resolved
packages/astro/src/core/pipeline.ts Outdated Show resolved Hide resolved
env: Readonly<Environment>,
mod: Readonly<ComponentInstance> | undefined,
onRequest?: MiddlewareHandler
async renderRoute(
Copy link
Member

Choose a reason for hiding this comment

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

I suppose the docs are outdated now, because it seems that this function doesn't throw any error anymore? We should update it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you know when that behavior changed?

packages/astro/src/core/pipeline.ts Outdated Show resolved Hide resolved
@lilnasy
Copy link
Contributor Author

lilnasy commented Feb 5, 2024

I can address the code style comments at the end of the review cycle. Do you have any comments on the overall direction.

To summarise, half of Pipeline's concerns are being rolled into Environment and the other half into RenderContext.

@ematipico
Copy link
Member

I can address the code style comments at the end of the review cycle. Do you have any comments on the overall direction.

To summarise, half of Pipeline's concerns are being rolled into Environment and the other half into RenderContext.

The direction seems reasonable to me; I have some thoughts:

  • now that we "lifted up" most of the things from the pipeline inside the environment, I think "environment" can be called pipeline, because that was the original idea I had envisioned. Maybe all the "static" things that should change can be put inside a static object
  • the createPipeline method, which returns a Pipeline doesn't have a clear meaning anymore. Maybe we can rename it RoutetRender, because that's what it does.
  • RouteCache should be something that only the dev enviroment/pipeline should have, they should not be part of the base enviroment/pipeline
  • The HiddenPipeline seems very cryptic, even when reading the code, documentation and its usage, it's very difficult to understand what it does

@lilnasy
Copy link
Contributor Author

lilnasy commented Feb 7, 2024

I think "environment" can be called pipeline, because that was the original idea I had envisioned

That's a good idea. RenderContext would be a more appropriate name for what's currently called Pipeline in this PR. RenderContext is intended to be the focus of the next and final refactor. We can wait until that one to merge this, because renaming the constructs multiple times will be confusing.

`environment.ts` is now lives directly in `src/core`, rather than `src/core/render`.

`environment.createPipeline` is removed. `Pipeline.create` is used instead.

Constructors with positional arguments are replaced by `Environment.create` with named arguments.

Clarifies the use of `HiddenPipeline`.
@lilnasy lilnasy merged commit 5acc313 into withastro:main Feb 20, 2024
13 checks passed
@lilnasy lilnasy deleted the refactor-pipeline-lifetime branch February 20, 2024 14:40
@astrobot-houston astrobot-houston mentioned this pull request Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants