fix: render state not unique per render

[marvinhagemeister]

This PR addresses a rendering correctness issue. In #1636 it was discovered that serialized island props were shared across renders when the server dealt with multiple requests at the same time. I did a bisect locally and the issue seems to be present since at least Fresh 1.0.

The problem

The gist of the problem is that we need to set up some global variables to track what Preact is rendering and to be aware of when we are trying to render an island so that we can inject markers in the HTML for the browser to continue from. This is fine in synchronous code and rendering with preact-render-to-string works that way anyway. Async route component don't change that, because they are not treated as a component, but rather a normal function that happens to return JSX.

Completely oversimplified the situation looked like this:

// This is a greatly oversimplified example, to demonstrate the issue.
// The actual code is more complex than this.

// Global to track which islands we rendered
let encounteredIslands = [];

async function renderAsyncPlugins(app, plugins) {
  for (const plugin of plugins) {
    // ...
  }
  
  return renderToString(app);
}

export function render(app, plugins) {
  // bad reset globals before async code
  encounteredIslands = [];
  
  // this calls synchronous `render()` from `preact-render-to-string` under the hood
  const html = await renderAsyncPlugins(app, plugins);
  
  // check which islands we rendered
  for (const islands of encounteredIslands) {
    // ...
  }
}

The problem is the async bit in between. There is no guarantee that you're dealing with the same global variables after the Promise is awaited. The globals are effectively leaked across renders. There were some problematic usage of globals similar to this scenario too.

The solution

The code has been rewritten so that you have to pass a rendering context around and cannot use globals. Instead of resetting the remaining globals at the start of the function and risk introducing asynchronicity later, that part has moved to be as close as possible to the renderToString call.

// Again, this is a greatly oversimplified example, to demonstrate the solution.
// The actual code is more complex than this.

// Global to track which islands we rendered
let encounteredIslands;

async function renderAsyncPlugins(app, plugins, renderState) {
  for (const plugin of plugins) {
    // ...
  }
  
  // Better: remaining globals moved as close as possible to the renderToString call
  try {
    encounteredIslands = renderState.encounteredIslands;
    return renderToString(app);
  } finally {
    // cleanup
    encounteredIslands = undefined;
  }
}

export function render(app, plugins) {
  const renderState = { ... }
  
  // this calls synchronous `render()` from `preact-render-to-string` under the hood
  const html = await renderAsyncPlugins(app, plugins, renderState);
  
  // check which islands we rendered, but now we access our render state instead
  for (const islands of renderState.encounteredIslands) {
    // ...
  }
}

Whilst the basic problem is simple to understand, the rendering code in Fresh relied on a lot of assumptions and used Preact's options.vnode-API incorrectly. It assumed that it would be called as part of the synchronous render pass, which there is no guarantee for. The only guarantee this hook gives is that it's called when a vnode is created. But vnodes can be created anywhere, not just during rendering. A simple example where a vnode is created outside of rendering:

// Created outside of rendering
const vnode = <div />

renderToString(vnode);

Longterm, the solution is to upstream the render state context API to preact-render-to-string, so that these kind of API mis-usages cannot happen and that no manual globals are needed.

In essence this PR therefore rewrites our whole rendering pipeline to get rid of any potential places to leak state across renders. This unfortunately makes the PR pretty big, but the new rendering code is much easier to follow now as a plus.

Fixes #1636

[bartlomieju]

LGTM, have you measure the performance impact of this change?

[bartlomieju]

This appears to still be global - won't this cause any problems?

[marvinhagemeister]

We cannot completely remove all globals unless we add some sort of render context API to Preact. This one here in particular is always replaced when a new rendered is triggered here https://github.com/denoland/fresh/pull/1638/files#diff-85a0f65cd2b2187632bf4c5c3c0385eae00d0d0cd49feac175ca59558b02490fR50