refactor: add pipeline concept #8020
Conversation
|
github-actions
bot
added
the
pkg: astro
| @@ -184,35 +184,8 @@ export class App { | |||
| } | |||
| Reflect.set(response, responseSentSymbol, true); | |||
| return response; | |||
| } else { | |||
There was a problem hiding this comment.
wow, love all of this deletion
There was a problem hiding this comment.
Agreed, all this cleanup in the App is great!
|
Looks great! Is the plan to later add a build pipeline and a dev pipeline? |
Yes. It's possible that we would add more "core functions" , or maybe not. So I want to make the changes slow and steady, so we get comfortable with the new flow of data and document things. |
| @@ -184,35 +184,8 @@ export class App { | |||
| } | |||
| Reflect.set(response, responseSentSymbol, true); | |||
| return response; | |||
| } else { | |||
There was a problem hiding this comment.
Agreed, all this cleanup in the App is great!
| onRequest?: MiddlewareEndpointHandler; | ||
| /** | ||
| * The handler accepts the *original* `Request` and result returned by the endpoint. | ||
| * It must return a `Response`. | ||
| */ | ||
| endpointHandler?: EndpointResultHandler; |
There was a problem hiding this comment.
Could we prefix these with # or mark them protected/private so we don't expose them?
| } | ||
|
|
||
| export class SSRRoutePipeline extends Pipeline { | ||
| encoder = new TextEncoder(); |
There was a problem hiding this comment.
Also maybe this could be private too.
| this.setEndpointHandler(this.#ssrEndpointHandler); | ||
| } | ||
|
|
||
| // This function is responsible for handling the result coming from an endpoint. | ||
| async #ssrEndpointHandler(request: Request, response: EndpointCallResult): Promise<Response> { |
There was a problem hiding this comment.
Small nit: Is this.setEndpointHandler and this.#ssrEndpointHandler something that every extended class of Pipeline needs to implement? I was thinking we can create an abstract method like abstract endpointHandler() in Pipeline instead, but I'm also fine with this pattern if you're still shaping up the final pattern.
There was a problem hiding this comment.
Technically, no.
Not all the future pipelines will have to handle endpoints. For example: a testing pipeline will need to be used only when needed, an Astro container won't need it (I presume)
Changes
This PR starts the refactors around how routes are rendered.
The idea is the following: creating a "core" that does one single thing, render a route by returning a
Response, and remove from the equation all the information that aren't part of a page. For example, theEnvironmentis not information that belongs exclusively to a page.Then, in the future refactos, we are going to create specialized pipelines - or consumers - that compute the basic information in a different way, and then handle the finished
Responsein a different way. These specialized pipelines will be: the dev server, the build, SSR. In the future we will have containers and testing.We are going to have a
Pipelinebase class, which exposes a method calledrenderRoute. It accepts theRenderContextand theComponentInstanceand returns aResponse. Very simple.In this PR I created the first consumer, AKA
SSRRoutePipeline. As you might have noticed, some logic was moved inside this new class.In order to make the internal developments better, I marked
routeinsideRenderContextmandatory. The reason why I did so is becauseRouteDatais supposed to contain the information of the type of the route we're about to render, but since could beundefined, we ended up having an API like this:Which doesn't make much sense, because we're supposed to have the
routealready insiderenderContext. I marked it mandatory, and the majority of the tests still pass, which means that the change in logic is correct.Testing
The existing tests should pass. I updated the tests where we create the
RenderContextmanually, because nowrouteis mandatory. The shape ofrouteused in tests won't trigger the function callscallGetStaticPaths, which isn't needed in those tests.Docs
/cc @withastro/maintainers-docs for feedback!