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

replace transformPage with transformPageChunk #5657

merged 5 commits into from
Jul 22, 2022

Conversation

Rich-Harris
Copy link
Member

Closes #4910.

This replaces the transformPage API with transformPageChunk, so that we can respond in a streaming fashion in future. We're not yet taking advantage of that ability, but updating the API is the first step.

Previously, transformPage took the entire HTML document:

export function handle({ event, resolve }) {
  return resolve(event, {
    transformPage: ({ html }) => html.replace('old', 'new')
  });
}

Now, it takes a chunk of HTML plus a done boolean. 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). Because of that, many transforms will work identically:

export function handle({ event, resolve }) {
  return resolve(event, {
-    transformPage: ({ html }) => html.replace('old', 'new')
+    transformPageChunk: ({ html }) => html.replace('old', 'new')
  });
}

In some cases, it's necessary to operate on the entire document in one go — for example, to validate and transform an AMP document. In these situations it's easy to buffer the HTML and return the transformed document at the end:

export function handle({ event, resolve }) {
  let buffer = '';
  return resolve(event, {
    transformPageChunk: ({ html, done }) => {
      buffer += html;
      if (done) return amp.transform(html);
    }
  });
}

Right now, done will always be true (i.e. there'll only be one chunk, which is the entire document) but that will change in the near future.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Jul 21, 2022

🦋 Changeset detected

Latest commit: 193a3a4

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

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

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

@benmccann
Copy link
Member

It looks like all the examples are buffering the whole page before dealing with it, so I wonder if we can't have a transformPage still just with the clearly documented caveat that if you use it, you lose streaming and buffer the whole page? I haven't looked into the implementation details to know if that's possible or not

@Rich-Harris
Copy link
Member Author

One of the examples (the AMP one) is buffering the whole page. The other one (html.replace('old', 'new')) is working on a chunk-by-chunk basis. (It so happens that there's only one chunk at present, but that's irrelevant.)

Having both APIs would be a mistake. Even if APIs were free, people would choose transformPage out of laziness and lose the benefits of streaming.

@@ -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

@Rich-Harris Rich-Harris merged commit ab65de0 into master Jul 22, 2022
@Rich-Harris Rich-Harris deleted the gh-4910 branch July 22, 2022 13:35
@accuser accuser mentioned this pull request Aug 9, 2022
5 tasks
brightowltutoring added a commit to brightowltutoring/sveltekit that referenced this pull request Dec 21, 2022
…cing 'transformPage' with 'transformPageChunk' as mentioned on 'sveltejs/kit#5657'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

replace transformPage with transformPageChunk or similar
3 participants