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: add pipeline concept #8020

Merged
merged 3 commits into from
Aug 10, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 21 additions & 49 deletions packages/astro/src/core/app/index.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import mime from 'mime';
import type {
EndpointHandler,
ManifestData,
MiddlewareEndpointHandler,
RouteData,
SSRElement,
SSRManifest,
} from '../../@types/astro';
import type { SinglePageBuiltModule } from '../build/types';
import { attachToResponse, getSetCookiesFromResponse } from '../cookies/index.js';
import { getSetCookiesFromResponse } from '../cookies/index.js';
import { consoleLogDestination } from '../logger/console.js';
import { error, type LogOptions } from '../logger/core.js';
import {
Expand All @@ -16,12 +16,10 @@ import {
removeTrailingForwardSlash,
} from '../path.js';
import { RedirectSinglePageBuiltModule } from '../redirects/index.js';
import { isResponse } from '../render/core.js';
import {
createEnvironment,
createRenderContext,
tryRenderRoute,
type Environment,
type RenderContext,
} from '../render/index.js';
import { RouteCache } from '../render/route-cache.js';
Expand All @@ -32,6 +30,7 @@ import {
} from '../render/ssr-element.js';
import { matchRoute } from '../routing/match.js';
import type { RouteInfo } from './types';
import { EndpointNotFoundError, SSRRoutePipeline } from './ssrPipeline.js';
export { deserializeManifest } from './common.js';

const clientLocalsSymbol = Symbol.for('astro.locals');
Expand All @@ -53,16 +52,15 @@ export class App {
/**
* The current environment of the application
*/
#env: Environment;
#manifest: SSRManifest;
#manifestData: ManifestData;
#routeDataToRouteInfo: Map<RouteData, RouteInfo>;
#encoder = new TextEncoder();
#logging: LogOptions = {
dest: consoleLogDestination,
level: 'info',
};
#baseWithoutTrailingSlash: string;
#pipeline: SSRRoutePipeline;

constructor(manifest: SSRManifest, streaming = true) {
this.#manifest = manifest;
Expand All @@ -71,7 +69,7 @@ export class App {
};
this.#routeDataToRouteInfo = new Map(manifest.routes.map((route) => [route.routeData, route]));
this.#baseWithoutTrailingSlash = removeTrailingForwardSlash(this.#manifest.base);
this.#env = this.#createEnvironment(streaming);
this.#pipeline = new SSRRoutePipeline(this.#createEnvironment(streaming));
}

set setManifest(newManifest: SSRManifest) {
Expand Down Expand Up @@ -163,19 +161,21 @@ export class App {
);
let response;
try {
response = await tryRenderRoute(
routeData.type,
renderContext,
this.#env,
pageModule,
mod.onRequest
);
// NOTE: ideally we could set the middleware function just once, but we don't have the infrastructure to that yet
if (mod.onRequest) {
this.#pipeline.setMiddlewareFunction(mod.onRequest as MiddlewareEndpointHandler);
}
response = await this.#pipeline.renderRoute(renderContext, pageModule);
} catch (err: any) {
error(this.#logging, 'ssr', err.stack || err.message || String(err));
return this.#renderError(request, { status: 500 });
if (err instanceof EndpointNotFoundError) {
return this.#renderError(request, { status: 404, response: err.originalResponse });
} else {
error(this.#logging, 'ssr', err.stack || err.message || String(err));
return this.#renderError(request, { status: 500 });
}
}

if (isResponse(response, routeData.type)) {
if (SSRRoutePipeline.isResponse(response, routeData.type)) {
if (STATUS_CODES.has(response.status)) {
return this.#renderError(request, {
response,
Expand All @@ -184,35 +184,8 @@ export class App {
}
Reflect.set(response, responseSentSymbol, true);
return response;
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

wow, love all of this deletion

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, all this cleanup in the App is great!

if (response.type === 'response') {
if (response.response.headers.get('X-Astro-Response') === 'Not-Found') {
return this.#renderError(request, {
response: response.response,
status: 404,
});
}
return response.response;
} else {
const headers = new Headers();
const mimeType = mime.getType(url.pathname);
if (mimeType) {
headers.set('Content-Type', `${mimeType};charset=utf-8`);
} else {
headers.set('Content-Type', 'text/plain;charset=utf-8');
}
const bytes =
response.encoding !== 'binary' ? this.#encoder.encode(response.body) : response.body;
headers.set('Content-Length', bytes.byteLength.toString());

const newResponse = new Response(bytes, {
status: 200,
headers,
});
attachToResponse(newResponse, response.cookies);
return newResponse;
}
}
return response;
}

setCookieHeaders(response: Response) {
Expand All @@ -238,7 +211,7 @@ export class App {
pathname,
route: routeData,
status,
env: this.#env,
env: this.#pipeline.env,
mod: handler as any,
});
} else {
Expand Down Expand Up @@ -272,7 +245,7 @@ export class App {
route: routeData,
status,
mod,
env: this.#env,
env: this.#pipeline.env,
});
}
}
Expand Down Expand Up @@ -301,9 +274,8 @@ export class App {
);
const page = (await mod.page()) as any;
const response = (await tryRenderRoute(
'page', // this is hardcoded to ensure proper behavior for missing endpoints
newRenderContext,
this.#env,
this.#pipeline.env,
page
)) as Response;
return this.#mergeResponses(response, originalResponse);
Expand Down
54 changes: 54 additions & 0 deletions packages/astro/src/core/app/ssrPipeline.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import type { Environment } from '../render';
import type { EndpointCallResult } from '../endpoint/index.js';
import mime from 'mime';
import { attachCookiesToResponse } from '../cookies/index.js';
import { Pipeline } from '../pipeline.js';

/**
* Thrown when an endpoint contains a response with the header "X-Astro-Response" === 'Not-Found'
*/
export class EndpointNotFoundError extends Error {
originalResponse: Response;
constructor(originalResponse: Response) {
super();
this.originalResponse = originalResponse;
}
}

export class SSRRoutePipeline extends Pipeline {
encoder = new TextEncoder();
Copy link
Member

Choose a reason for hiding this comment

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

Also maybe this could be private too.


constructor(env: Environment) {
super(env);
this.setEndpointHandler(this.#ssrEndpointHandler);
}

// This function is responsible for handling the result coming from an endpoint.
async #ssrEndpointHandler(request: Request, response: EndpointCallResult): Promise<Response> {
Comment on lines +23 to +27
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

if (response.type === 'response') {
if (response.response.headers.get('X-Astro-Response') === 'Not-Found') {
throw new EndpointNotFoundError(response.response);
}
return response.response;
} else {
const url = new URL(request.url);
const headers = new Headers();
const mimeType = mime.getType(url.pathname);
if (mimeType) {
headers.set('Content-Type', `${mimeType};charset=utf-8`);
} else {
headers.set('Content-Type', 'text/plain;charset=utf-8');
}
const bytes =
response.encoding !== 'binary' ? this.encoder.encode(response.body) : response.body;
headers.set('Content-Length', bytes.byteLength.toString());

const newResponse = new Response(bytes, {
status: 200,
headers,
});
attachCookiesToResponse(newResponse, response.cookies);
return newResponse;
}
}
}
2 changes: 1 addition & 1 deletion packages/astro/src/core/build/generate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,7 @@ async function generatePath(

let response;
try {
response = await tryRenderRoute(pageData.route.type, renderContext, env, mod, onRequest);
response = await tryRenderRoute(renderContext, env, mod, onRequest);
} catch (err) {
if (!AstroError.is(err) && !(err as SSRError).id && typeof err === 'object') {
(err as SSRError).id = pageData.component;
Expand Down
2 changes: 1 addition & 1 deletion packages/astro/src/core/cookies/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
export { AstroCookies } from './cookies.js';
export { attachToResponse, getSetCookiesFromResponse } from './response.js';
export { attachCookiesToResponse, getSetCookiesFromResponse } from './response.js';
2 changes: 1 addition & 1 deletion packages/astro/src/core/cookies/response.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type { AstroCookies } from './cookies';

const astroCookiesSymbol = Symbol.for('astro.cookies');

export function attachToResponse(response: Response, cookies: AstroCookies) {
export function attachCookiesToResponse(response: Response, cookies: AstroCookies) {
Reflect.set(response, astroCookiesSymbol, cookies);
}

Expand Down
4 changes: 2 additions & 2 deletions packages/astro/src/core/endpoint/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import type {
import type { Environment, RenderContext } from '../render/index';
import { renderEndpoint } from '../../runtime/server/index.js';
import { ASTRO_VERSION } from '../constants.js';
import { AstroCookies, attachToResponse } from '../cookies/index.js';
import { AstroCookies, attachCookiesToResponse } from '../cookies/index.js';
import { AstroError, AstroErrorData } from '../errors/index.js';
import { warn } from '../logger/core.js';
import { callMiddleware } from '../middleware/callMiddleware.js';
Expand Down Expand Up @@ -125,7 +125,7 @@ export async function callEndpoint<MiddlewareResult = Response | EndpointOutput>
}

if (response instanceof Response) {
attachToResponse(response, context.cookies);
attachCookiesToResponse(response, context.cookies);
return {
type: 'response',
response,
Expand Down
Loading