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 or similar #4910

Closed
Rich-Harris opened this issue May 13, 2022 · 1 comment · Fixed by #5657
Closed

replace transformPage with transformPageChunk or similar #4910

Rich-Harris opened this issue May 13, 2022 · 1 comment · Fixed by #5657
Labels
breaking change p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc.
Milestone

Comments

@Rich-Harris
Copy link
Member

Describe the problem

SvelteKit should have the ability to stream HTML. That could take several forms:

  1. We immediately flush the contents of src/app.html up to %svelte.head%, plus the <link> elements that preload .js and .css files for the page (which we could determine as soon as the route is matched with some fairly minor changes), then flush the rest all in one go once the page is done rendering. This improves TTFB and lets the browser get a head start on fetching resources
  2. We immediately flush the top (as in 1), then render a placeholder element for any <svelte:head> content discovered during rendering, then flush up to %svelte.body%, then flush the rest once the page renders
  3. We do 2, but instead of rendering the whole body synchronously, we render layouts as soon as they're ready, meaning that in many cases we could flush a <nav> bar etc before the main content is ready

There's also a version where Svelte itself supports streaming rendering, and we pause on {#await ...} blocks, but that's a far more involved conversation that we don't need to have today.

In fact, we don't need to decide any of this stuff today; we just need to identify the things that block us from implementing these ideas. 3 is impossible as long as we have $page.stuff (more on that in a separate issue), but all three versions are impossible as long as we have the transformPage API since it expects the entire page to be buffered.

Describe the proposed solution

Instead of transformPage, we could have something like transformPageChunk or transformMarkup which operates on a chunk at a time. In the common case, it would behave exactly the same:

export async function handle({ event, resolve }) {
  return resolve(event, {
    transformMarkup: ({ html }) => html.replace(/cloud/g, 'butt')
  });
}

We can guarantee that the chunks are sensible (the entirety of a layout up to the <slot/> or following it, for example) so that it's not necessary to handle the case where one chunk contains 'clo' and the next contains 'ud'. That said, in some rare cases it might be necessary to buffer content, which means we would need to include a flag for the final chunk:

export async function handle({ event, resolve }) {
  let buffer = '';
  return resolve(event, {
    transformMarkup: ({ html, last }) => {
      if (last) return minify(buffer + html);
      else buffer += html;
    }
  });
}

(Realistically, you'd probably just disable streaming in those situations, but last costs nothing.)

Until streaming is implemented (which probably won't happen straight away as it involves lots of decisions — is it driven by config? by a resolve option? do we do version 1, 2, or 3?) this function would behave exactly the same way as transformPage currently does.

Alternatives considered

Keep the current API, but opt out of streaming when transformPage is provided. I think that would be a mistake; there are lots of valid uses for transforming markup that are no less valid in a streaming context.

Importance

would make my life easier

Additional Information

Aside: streaming implies everything gets a 200 response except early 404s. Apparently the way to make this not matter is to inject a <meta name="robots" content="noindex"> into the page — doesn't matter where — if you encounter an error during rendering.

Another aside: out-of-order streaming (as suggested in 2, where we inject a placeholder element for <svelte:head> content, then render snippets of code that inject the HTML into the head once it's ready) relies on JavaScript. It should therefore a) be behind an option, and b) be disabled for search engine bots, since reliance on JS is bad for SEO despite some alarmingly common misconceptions. (We would probably need to make the list of bot UAs configurable.)

@Rich-Harris
Copy link
Member Author

Should have mentioned: there's a discussion around this here: #4831

@Rich-Harris Rich-Harris added this to the 1.0 milestone May 13, 2022
@benmccann benmccann added p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc. breaking change labels Jul 19, 2022
Rich-Harris added a commit that referenced this issue Jul 22, 2022
* replace transformPage with transformPageChunk - closes #4910

* Update packages/kit/src/runtime/server/index.js

* fix type

* fix

* update docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants