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

Adding an option to disable HTTP streaming #3777

Merged
merged 18 commits into from
Jul 1, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
5 changes: 5 additions & 0 deletions .changeset/curly-worms-agree.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@astrojs/cloudflare': patch
---

Disables HTTP streaming in Cloudflare Pages deployments
5 changes: 5 additions & 0 deletions .changeset/two-dolls-marry.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Adds an option to disable HTTP streaming in Astro's production `App` server
5 changes: 4 additions & 1 deletion packages/astro/src/core/app/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,16 @@ export class App {
dest: consoleLogDestination,
level: 'info',
};
#streaming: boolean;

constructor(manifest: Manifest) {
constructor(manifest: Manifest, streaming = true) {
this.#manifest = manifest;
this.#manifestData = {
routes: manifest.routes.map((route) => route.routeData),
};
this.#routeDataToRouteInfo = new Map(manifest.routes.map((route) => [route.routeData, route]));
this.#routeCache = new RouteCache(this.#logging);
this.#streaming = streaming;
}
match(request: Request): RouteData | undefined {
const url = new URL(request.url);
Expand Down Expand Up @@ -117,6 +119,7 @@ export class App {
site: this.#manifest.site,
ssr: true,
request,
streaming: this.#streaming,
});

return response;
Expand Down
1 change: 1 addition & 0 deletions packages/astro/src/core/build/generate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,7 @@ async function generatePath(
? new URL(astroConfig.base, astroConfig.site).toString()
: astroConfig.site,
ssr,
streaming: true,
};

let body: string;
Expand Down
2 changes: 2 additions & 0 deletions packages/astro/src/core/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ const ASTRO_CONFIG_DEFAULTS: AstroUserConfig & any = {
server: {
host: false,
port: 3000,
streaming: true,
tony-sull marked this conversation as resolved.
Show resolved Hide resolved
},
style: { postcss: { options: {}, plugins: [] } },
integrations: [],
Expand Down Expand Up @@ -315,6 +316,7 @@ export async function validateConfig(
.optional()
.default(ASTRO_CONFIG_DEFAULTS.server.host),
port: z.number().optional().default(ASTRO_CONFIG_DEFAULTS.server.port),
streaming: z.boolean().optional().default(true),
})
.optional()
.default({})
Expand Down
5 changes: 4 additions & 1 deletion packages/astro/src/core/render/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ export interface RenderOptions {
routeCache: RouteCache;
site?: string;
ssr: boolean;
streaming: boolean;
request: Request;
}

Expand All @@ -100,6 +101,7 @@ export async function render(opts: RenderOptions): Promise<Response> {
routeCache,
site,
ssr,
streaming,
} = opts;

const paramsAndPropsRes = await getParamsAndProps({
Expand Down Expand Up @@ -138,12 +140,13 @@ export async function render(opts: RenderOptions): Promise<Response> {
site,
scripts,
ssr,
streaming,
});

// Support `export const components` for `MDX` pages
if (typeof (mod as any).components === 'object') {
Object.assign(pageProps, { components: (mod as any).components });
}

return await renderPage(result, Component, pageProps, null);
return await renderPage(result, Component, pageProps, null, streaming);
}
1 change: 1 addition & 0 deletions packages/astro/src/core/render/dev/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ export async function render(
routeCache,
site: astroConfig.site ? new URL(astroConfig.base, astroConfig.site).toString() : undefined,
ssr: isBuildingToSSR(astroConfig),
streaming: true,
});

return response;
Expand Down
7 changes: 6 additions & 1 deletion packages/astro/src/core/render/result.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ function onlyAvailableInSSR(name: string) {

export interface CreateResultArgs {
ssr: boolean;
streaming: boolean;
logging: LogOptions;
origin: string;
markdown: MarkdownRenderingOptions;
Expand Down Expand Up @@ -114,7 +115,11 @@ export function createResult(args: CreateResultArgs): SSRResult {
const url = new URL(request.url);
const canonicalURL = createCanonicalURL('.' + pathname, site ?? url.origin, paginated);
const headers = new Headers();
headers.set('Transfer-Encoding', 'chunked');
if (args.streaming) {
headers.set('Transfer-Encoding', 'chunked');
} else {
headers.set('Content-Type', 'text/html');
}
const response: ResponseInit = {
status: 200,
statusText: 'OK',
Expand Down
59 changes: 40 additions & 19 deletions packages/astro/src/runtime/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -704,7 +704,8 @@ export async function renderPage(
result: SSRResult,
componentFactory: AstroComponentFactory,
props: any,
children: any
children: any,
streaming: boolean,
): Promise<Response> {
let iterable: AsyncIterable<any>;
if (!componentFactory.isAstroComponentFactory) {
Expand All @@ -730,28 +731,48 @@ export async function renderPage(
const factoryReturnValue = await componentFactory(result, props, children);

if (isAstroComponent(factoryReturnValue)) {
iterable = renderAstroComponent(factoryReturnValue);
let stream = new ReadableStream({
start(controller) {
async function read() {
let i = 0;
for await (const chunk of iterable) {
let html = chunk.toString();
if (i === 0) {
if (!/<!doctype html/i.test(html)) {
controller.enqueue(encoder.encode('<!DOCTYPE html>\n'));
let iterable = renderAstroComponent(factoryReturnValue);
let init = result.response;
let headers = new Headers(init.headers);
let body: BodyInit;
if (streaming) {
Copy link
Member

Choose a reason for hiding this comment

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

Bah, I wish CF supported ReadableStream. I'm still on a mission to reduce forking code paths, so in that spirit I'd love to use ReadableStream in both cases and then just pipe it to a string as a smaller, additional step in the non-streaming code path.

I can't think of any good way to remove the copy-paste code between the two if-else code paths, so I guess this might not be actionable. Just venting I guess :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was actually my first solution! We can't use ReadableStream in CF at all unfortunately, the constructor throws an error immediately. Even worse, the error is basically silent if you don't add an extra try/catch around the entire SSR handler and logs are accessible yet for Cloudflare Pages Functions 🙃

body = new ReadableStream({
start(controller) {
async function read() {
let i = 0;
for await (const chunk of iterable) {
let html = chunk.toString();
if (i === 0) {
if (!/<!doctype html/i.test(html)) {
controller.enqueue(encoder.encode('<!DOCTYPE html>\n'));
}
}
controller.enqueue(encoder.encode(html));
i++;
}
controller.enqueue(encoder.encode(html));
i++;
controller.close();
}
read();
},
});
} else {
body = '';
let i = 0;
for await (const chunk of iterable) {
let html = chunk.toString();
if (i === 0) {
if (!/<!doctype html/i.test(html)) {
body += '<!DOCTYPE html>\n';
}
controller.close();
}
read();
},
});
let init = result.response;
let response = createResponse(stream, init);
body += chunk;
i++;
}
const bytes = encoder.encode(body);
headers.set('Content-Length', `${bytes.byteLength}`);
}

let response = createResponse(body, { ...init, headers });
return response;
} else {
return factoryReturnValue;
Expand Down
3 changes: 3 additions & 0 deletions packages/astro/src/runtime/server/response.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ type CreateResponseFn = (body?: BodyInit | null, init?: ResponseInit) => Respons

export const createResponse: CreateResponseFn = isNodeJS
? (body, init) => {
if (typeof body === 'string') {
return new Response(body, init);
}
if (typeof StreamingCompatibleResponse === 'undefined') {
return new (createResponseClass())(body, init);
}
Expand Down
2 changes: 2 additions & 0 deletions packages/astro/src/vite-plugin-astro-server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,8 @@ async function writeWebResponse(res: http.ServerResponse, webResponse: Response)
} else if (body instanceof Readable) {
body.pipe(res);
return;
} else if (typeof body === 'string') {
res.write(body);
} else {
const reader = body.getReader();
while (true) {
Expand Down
69 changes: 69 additions & 0 deletions packages/astro/test/streaming.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,3 +71,72 @@ describe('Streaming', () => {
});
});
});

describe('Streaming disabled', () => {
if (isWindows) return;

/** @type {import('./test-utils').Fixture} */
let fixture;

before(async () => {
fixture = await loadFixture({
root: './fixtures/streaming/',
adapter: testAdapter(),
experimental: {
ssr: true,
},
server: {
streaming: false,
}
});
});

describe('Development', () => {
/** @type {import('./test-utils').DevServer} */
let devServer;

before(async () => {
devServer = await fixture.startDevServer();
});

after(async () => {
await devServer.stop();
});

it('Body is chunked', async () => {
let res = await fixture.fetch('/');
let chunks = [];
for await (const bytes of res.body) {
let chunk = bytes.toString('utf-8');
chunks.push(chunk);
}
expect(chunks.length).to.be.greaterThan(1);
});
});

// TODO: find a different solution for the test-adapter,
// currently there's no way to resolve two different versions with one
// having streaming disabled
describe('Production', () => {
before(async () => {
await fixture.build();
});

it('Can get the full html body', async () => {
const app = await fixture.loadTestAdapterApp(false);
const request = new Request('http://example.com/');
const response = await app.render(request);

expect(response.status).to.equal(200);
expect(response.headers.get('content-type')).to.equal('text/html');
expect(response.headers.has('content-length')).to.equal(true);
expect(parseInt(response.headers.get('content-length'))).to.be.greaterThan(0);

const html = await response.text();
const $ = cheerio.load(html);

expect($('header h1')).to.have.a.lengthOf(1);
expect($('ul li')).to.have.a.lengthOf(10);
});
});
});
2 changes: 1 addition & 1 deletion packages/astro/test/test-adapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export default function () {
},
load(id) {
if (id === '@my-ssr') {
return `import { App } from 'astro/app';export function createExports(manifest) { return { manifest, createApp: () => new App(manifest) }; }`;
return `import { App } from 'astro/app';export function createExports(manifest) { return { manifest, createApp: (streaming) => new App(manifest, streaming) }; }`;
}
},
},
Expand Down
4 changes: 2 additions & 2 deletions packages/astro/test/test-utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -149,10 +149,10 @@ export async function loadFixture(inlineConfig) {
clean: async () => {
await fs.promises.rm(config.outDir, { maxRetries: 10, recursive: true, force: true });
},
loadTestAdapterApp: async () => {
loadTestAdapterApp: async (streaming) => {
const url = new URL('./server/entry.mjs', config.outDir);
const { createApp, manifest } = await import(url);
const app = createApp();
const app = createApp(streaming);
app.manifest = manifest;
return app;
},
Expand Down
2 changes: 1 addition & 1 deletion packages/integrations/cloudflare/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ type Env = {
};

export function createExports(manifest: SSRManifest) {
const app = new App(manifest);
const app = new App(manifest, false);

const fetch = async (request: Request, env: Env) => {
const { origin, pathname } = new URL(request.url);
Expand Down
9 changes: 9 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.