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

replace transformPage with transformPageChunk #5657

Merged
merged 5 commits into from
Jul 22, 2022
Merged
Show file tree
Hide file tree
Changes from 4 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/nasty-seahorses-know.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

[breaking] replace transformPage with transformPageChunk
4 changes: 2 additions & 2 deletions documentation/docs/06-hooks.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,15 +67,15 @@ You can add call multiple `handle` functions with [the `sequence` helper functio
`resolve` also supports a second, optional parameter that gives you more control over how the response will be rendered. That parameter is an object that can have the following fields:

- `ssr: boolean` (default `true`) — if `false`, renders an empty 'shell' page instead of server-side rendering
- `transformPage(opts: { html: string }): string` — applies custom transforms to HTML
- `transformPageChunk(opts: { html: string, done: boolean }): string` — applies custom transforms to HTML. If `done` is true, it's the final chunk
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if there's anything we should say here, but I'm not that familiar with how the chunking happens. Does it match a single write to the stream? Or is it split up so that each chunk is less than the TCP window size or something? E.g. I'm wondering if there's any risk that the .replace('old', 'new') example doesn't work because "ol" is one chunk and "d" in the next chunk

Copy link
Member Author

Choose a reason for hiding this comment

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

I addressed that above:

Chunks are not guaranteed to be well-formed HTML (they might include the opening tag of an element but not its closing tag, for example) but nor are they arbitrary — they will always be split at sensible boundaries (like %sveltekit.[property]% or before the contents of a layout or page component)

I figured there wasn't much point going into detail in the docs until we actually start streaming responses

Copy link
Member

Choose a reason for hiding this comment

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

I guess I should read PR descriptions 😄 Still, it might be worth including here so that people write their code correctly from the beginning and so that we don't break their applications when we do start streaming

Copy link
Member Author

Choose a reason for hiding this comment

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

added some detail


```js
/// file: src/hooks.js
/** @type {import('@sveltejs/kit').Handle} */
export async function handle({ event, resolve }) {
const response = await resolve(event, {
ssr: !event.url.pathname.startsWith('/admin'),
transformPage: ({ html }) => html.replace('old', 'new')
transformPageChunk: ({ html }) => html.replace('old', 'new')
});

return response;
Expand Down
8 changes: 6 additions & 2 deletions documentation/docs/17-seo.md
Original file line number Diff line number Diff line change
Expand Up @@ -105,15 +105,19 @@ const config = {
export default config;
```

...and transforming the HTML using `transformPage` along with `transform` imported from `@sveltejs/amp`:
...and transforming the HTML using `transformPageChunk` along with `transform` imported from `@sveltejs/amp`:

```js
import * as amp from '@sveltejs/amp';

/** @type {import('@sveltejs/kit').Handle} */
export async function handle({ event, resolve }) {
let buffer = '';
return resolve(event, {
transformPage: ({ html }) => amp.transform(html)
transformPageChunk: ({ html, done }) => {
buffer += html;
if (done) return amp.transform(html);
}
});
}
```
Expand Down
12 changes: 10 additions & 2 deletions packages/kit/src/runtime/server/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ export async function respond(request, options, state) {
/** @type {import('types').RequiredResolveOptions} */
let resolve_opts = {
ssr: true,
transformPage: default_transform
transformPageChunk: default_transform
};

// TODO match route before calling handle?
Expand All @@ -181,9 +181,17 @@ export async function respond(request, options, state) {
event,
resolve: async (event, opts) => {
if (opts) {
// TODO remove for 1.0
// @ts-expect-error
if (opts.transformPage) {
throw new Error(
'transformPage has been replaced by transformPageChunk — see https://github.com/sveltejs/kit/pull/5657 for more information'
);
}

resolve_opts = {
ssr: opts.ssr !== false,
transformPage: opts.transformPage || default_transform
transformPageChunk: opts.transformPageChunk || default_transform
};
}

Expand Down
9 changes: 6 additions & 3 deletions packages/kit/src/runtime/server/page/render.js
Original file line number Diff line number Diff line change
Expand Up @@ -291,9 +291,12 @@ export async function render_response({
const assets =
options.paths.assets || (segments.length > 0 ? segments.map(() => '..').join('/') : '.');

const html = await resolve_opts.transformPage({
html: options.template({ head, body, assets, nonce: /** @type {string} */ (csp.nonce) })
});
// TODO flush chunks as early as we can
const html =
(await resolve_opts.transformPageChunk({
html: options.template({ head, body, assets, nonce: /** @type {string} */ (csp.nonce) }),
done: true
})) || '';

const headers = new Headers({
'content-type': 'text/html',
Expand Down
32 changes: 19 additions & 13 deletions packages/kit/test/apps/amp/src/hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,28 @@ import * as amp from '@sveltejs/amp';

/** @type {import('@sveltejs/kit').Handle} */
export async function handle({ event, resolve }) {
let buffer = '';

const response = await resolve(event, {
transformPage: ({ html }) => {
html = amp.transform(html);
transformPageChunk: ({ html, done }) => {
buffer += html;

if (done) {
const html = amp.transform(buffer);

// remove unused CSS
let css = '';
const markup = html.replace(
/<style amp-custom([^>]*?)>([^]+?)<\/style>/,
(match, attributes, contents) => {
css = contents;
return `<style amp-custom${attributes}></style>`;
}
);
// remove unused CSS
let css = '';
const markup = html.replace(
/<style amp-custom([^>]*?)>([^]+?)<\/style>/,
(match, attributes, contents) => {
css = contents;
return `<style amp-custom${attributes}></style>`;
}
);

css = purify(markup, css);
return markup.replace('</style>', `${css}</style>`);
css = purify(markup, css);
return markup.replace('</style>', `${css}</style>`);
}
}
});

Expand Down
2 changes: 1 addition & 1 deletion packages/kit/test/apps/basics/src/hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ export const handle = sequence(

const response = await resolve(event, {
ssr: !event.url.pathname.startsWith('/no-ssr'),
transformPage: event.url.pathname.startsWith('/transform-page')
transformPageChunk: event.url.pathname.startsWith('/transform-page-chunk')
? ({ html }) => html.replace('__REPLACEME__', 'Worked!')
: undefined
});
Expand Down
4 changes: 2 additions & 2 deletions packages/kit/test/apps/basics/test/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1009,8 +1009,8 @@ test.describe('Page options', () => {
}
});

test('transformPage can change the html output', async ({ page }) => {
await page.goto('/transform-page');
test('transformPageChunk can change the html output', async ({ page }) => {
await page.goto('/transform-page-chunk');
expect(await page.getAttribute('meta[name="transform-page"]', 'content')).toBe('Worked!');
});

Expand Down
7 changes: 4 additions & 3 deletions packages/kit/test/prerendering/basics/src/hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,14 @@ import { prerendering } from '$app/env';

const initial_prerendering = prerendering;

/** @type {import('@sveltejs/kit').Handle} */
export const handle = async ({ event, resolve }) => {
if (event.url.pathname === '/prerendering-true' && prerendering) {
return await resolve(event, {
transformPage: ({ html }) =>
transformPageChunk: ({ html }) =>
html
.replace('__INITIAL_PRERENDERING__', initial_prerendering)
.replace('__PRERENDERING__', prerendering)
.replace('__INITIAL_PRERENDERING__', String(initial_prerendering))
.replace('__PRERENDERING__', String(prerendering))
});
}
return await resolve(event);
Expand Down
2 changes: 1 addition & 1 deletion packages/kit/types/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ export interface RequestHandlerOutput<Output = ResponseBody> {

export interface ResolveOptions {
ssr?: boolean;
transformPage?: ({ html }: { html: string }) => MaybePromise<string>;
transformPageChunk?: (input: { html: string; done: boolean }) => MaybePromise<string | undefined>;
}

export type ResponseBody = JSONValue | Uint8Array | ReadableStream | Error;
Expand Down