API suggestion: deprecate rewind() and use a closure to prevent leaks and race conditions

[ide]

This is a proposal to deprecate the current API for server-side rendering:

let markup = ReactDOM.renderToString(<App />);
let title = DocumentTitle.rewind();

and replace it with this, inspired by Aphrodite's SSR API:

let { title, markup } = DocumentTitle.renderStatic(
  () => ReactDOM.renderToString(<App />)
);

This API has two advantages. It is impossible to forget to call rewind. It is also easy to throw an error if DocumentTitle.renderStatic is called in a reentrant way.

You could use this with Aphrodite easily too. The markup field in the result is whatever you return from the closure (can figure out a better name later -- I just didn't want to call it "html" since it might include CSS):

let { title, markup: { html, css } } = DocumentTitle.renderStatic(
  // Aphrodite.renderStatic() returns { html, css }
  () => Aphrodite.renderStatic(() => ReactDOM.renderToString(<App />))
);

This is easy to build in user-space but I think it might be a strictly better API than rewind() so wanted to start a discussion here.

[ide]

FWIW I wrote a wrapper for react-helmet that does this:

import assert from 'assert';
import Helmet from 'react-helmet';

let isRendering = false;

function renderStatic(render) {
  assert(
    !isRendering,
    `Must not call DocumentHead.renderStatic when already rendering an element`,
  );

  let head;
  let result;
  isRendering = true;
  try {
    result = render();
  } finally {
    head = Helmet.rewind();
    isRendering = false;
  }

  return { head, result };
}

export default { renderStatic };

[tizmagik]

I like this. Could this also be opt-in so that .rewind() is still possible?

[lourd]

Are y'all still interested in this feature?

After reading @amannn's comment in react-helmet, I understand why the rewind approach could be a shortcoming for some users, with the possible underlying race condition for parallel renders. The proposed API wouldn't resolve that, though. You would need to make separate side-effect wrapped components for each render/request to keep the internal state separate.

[kanzelm3]

+1

[ide]

Yeah, I think this is mostly independent from supporting concurrent rendering but writing out some hypothetical APIs (especially since React Fiber makes concurrent rendering more relevant) has shifted my opinion a bit.

The closure API would go from:

DocumentTitle.renderStatic(
  () => ReactDOM.renderToString(<App />)
)

and could receive a context value:

// May be time-sliced over multiple rIC/rAF/microtask ticks
DocumentTitle.renderStaticAsync(
  async (sideEffectState) => ReactDOM.renderToStringWithFiberScheduling(
    // Haven't really thought about what goes on in here, probably something with React context
    <App documentTitleSideEffectState={sideEffectState} />
  )
)

In contrast, the rewind() API instead would look look like:

let sideEffectState = DocumentTitle.createSideEffectState();
await ReactDOM.renderToStringWithFiberScheduling(
  <App documentTitleSideEffectState={sideEffectState} />
);
// No more rewind() since all state is in sideEffectState, which gets garbage collected

I kind of like the simplicity of the latter API. I think if react-side-effect were to make a breaking change to its API and require explicitly using sideEffectState and remove rewind(), I'd prefer the latter API. It's clearer to see when different parts of the code run and you can't mess it up -- you can't silently forget to call createSideEffectState() the same way you can forget to call rewind().